Upload file #242

boyska · 2016-02-05T00:00:49Z

This starts where #209 has left us: a quite-working but hard to read code.
In the last commit, I try to make it slightly better.
Please remind me other blockers I have to fix about the previous code.

ael-code · 2016-02-17T17:44:14Z

It is not clear from the command help message how should I pass the metadata of the volume that I'm adding.

Anyway, the following command

libreant-db insert-volume -l "en"

raises:

2016-02-17 11:42:47,419 [archivant] [WARNING] It has not been set any value for FSDB_PATH, file operations will not be supported
Traceback (most recent call last):
  File "/home/libreant/libreant/ve/bin/libreant-db", line 9, in <module>
    load_entry_point('libreant==0.3', 'console_scripts', 'libreant-db')()
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/home/libreant/libreant/cli/libreant_db.py", line 129, in insert_volume
    meta.update(json.loads(metadata))
  File "/usr/lib/python2.7/json/__init__.py", line 338, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 366, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
TypeError: expected string or buffer

boyska · 2016-02-17T18:19:18Z

thanks @ael-code that version actually had a small but grave bug about metadata reading. I think that it has been fixed now.

About documentation, I think that it is much clearer now: I have added a huge docstring complete with examples. Yes, those may fit better the sphinx documentation or, even better, manpages (see #234 ), but while we wait for those...

ael-code · 2016-02-28T09:42:46Z

~ libreant-db insert-volume -l en '{"title": "prova"}'                           :(

2016-02-28 03:40:58,204 [archivant] [WARNING] It has not been set any value for FSDB_PATH, file operations will not be supported
Usage: libreant-db insert-volume [OPTIONS] METADATA

Error: Invalid value for "metadata": Could not open file: {"title": "prova"}: No such file or directory

boyska · 2016-02-28T15:26:06Z

@ael-code I think that it is clear enough in boyska@53d02e8#diff-9ff44f258347a5353864b0115b6c2894R130
that the argument must be a path to a file containing that json data. I thought that the --help this time was really really clear, but maybe it is not.

ael-code · 2016-02-28T15:30:29Z

@ael-code I think that it is clear enough in boyska/libreant@53d02e8#diff-9ff44f258347a5353864b0115b6c2894R130
that the argument must be a path to a file containing that json data. I thought that the --help this time was really really clear, mut baybe it is not.

I forgot about it. Sorry man :)

ael-code · 2016-02-28T15:58:01Z

there is a missing - on error message:

The number of --filepath, --names, and -note should be the same

Moreover I did not understood why --names is the only one that make use of the plural form.

Anyway in the REST API if you omit the param name it will be taken from the name of the file itself.

ael-code · 2016-02-28T16:00:00Z

I cannot attach a file without an extension :|

boyska · 2016-02-28T16:40:32Z

ael wrote:

I cannot attach a file without an extension :|

this seems a bug. But, what files have you tried with? My guess is that
if you have some file whose encoding is not easy to detect (or maybe is
just empty!) then a failure is not a huge problem, because it's true
that the mimetype is unknown. If, however, the mimetype should be
detected, then it is a serious bug.

boyska · 2016-03-03T18:42:01Z

@ael-code I inspected the extension thing better. python builtin mimetypes module needs extension to be used, and relies only on them. If we want to extract it looking at the content, we could use python-magic, which relies on libmagic, the same library used by the file command. I don't think that this is a blocking requirement, but if you think so, I am ready to add support for it.

ael-code · 2016-03-04T10:36:26Z

I don't think that this is a blocking requirement, but if you think so, I am ready to add support for it.

I'm going to repeat myself:
I cannot attach a file without an extension

Are you working for Microsoft or what? :)

boyska · 2016-03-04T14:09:48Z

ael:

I don't think that this is a blocking requirement, but if you think so, I am ready to add support for it.

I'm going to repeat myself:
I cannot attach a file without an extension

when you get old, you often repeat yourself :P

Are you working for Microsoft or what? :)

ahaha. So python-magic will be? fine for me.

boyska

ael-code · 2016-03-05T19:07:53Z

On 03/04/2016 03:09 PM, BoySka wrote:

ael:

I don't think that this is a blocking requirement, but if you think
so, I am ready to add support for it.

I'm going to repeat myself:
I cannot attach a file without an extension

when you get old, you often repeat yourself :P

Are you working for Microsoft or what? :)

ahaha. So python-magic will be? fine for me.

I think that we are going to far with this.

According to the archivant api [1] we've decided that mime-type and
notes are not mandatory. And the name of the file is optional if you
provide a real path to extract it from.

I don't know an easy way to do it, but I will make the cli reflects the
choiches taken for archivant api.

Otherwise, if you think that mime-type should be mandatory we should
change archivant interface as well.

[1]
http://libreant.readthedocs.org/en/latest/api/archivant.html#archivant.Archivant.insert_volume

boyska · 2016-03-05T19:20:22Z

ael:

I think that we are going too far with this.

According to the archivant api [1] we've decided that mime-type and
notes are not mandatory. And the name of the file is optional if you
provide a real path to extract it from.

I don't know an easy way to do it, but I will make the cli reflects the
choiches taken for archivant api.

your point is correct.

Otherwise, if you think that mime-type should be mandatory we should
change archivant interface as well.

nope, you're right. So I will:

still try to extract mime-type from filename, but if it fails just go
ahead
stop asking the filename, since it's obviously extracted from the path
still asking for notes, because making it optional is too difficult
and removing it does not feel good

boyska · 2016-03-10T02:50:41Z

I did everything I promised in the previous comment

ael-code · 2016-03-10T20:43:30Z

Looking at the examples in the help message:

Adds a volume with one attachment but no metadata
libreant_db insert-volume -l en -f /path/book.epub --notes 'poor quality'
Adds a volume with two attachments but no metadata
`libreant_db insert-volume -l en -f /path/book.epub --notes 'poor quality' -f /path/someother.epub --notes 'preprint'``

Both the above examples do not work because metadata are missing, in fact they both raise:
Error: Missing argument "metadata".

Then if I pass all parameters correctly and a regular file without extension as attachment an exception is raised:

Traceback (most recent call last):
  File "/home/libreant/libreant/ve/bin/libreant-db", line 9, in <module>
    load_entry_point('libreant==0.3', 'console_scripts', 'libreant-db')()
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click-6.3-py2.7.egg/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click-6.3-py2.7.egg/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click-6.3-py2.7.egg/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click-6.3-py2.7.egg/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/libreant/libreant/ve/local/lib/python2.7/site-packages/click-6.3-py2.7.egg/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/home/libreant/libreant/cli/libreant_db.py", line 153, in insert_volume
    attachments = attach_list(filepath, notes)
  File "/home/libreant/libreant/cli/libreant_db.py", line 181, in attach_list
    if '/' not in mime:
TypeError: argument of type 'NoneType' is not iterable

Naming and typos

In the help message you use libreant_db instead of libreant_db for the command name.
the language is about the volume in general and not the media

  -l, --language TEXT  specify the language of the media you are going to
                       upload  [required]

Usually we use attachment for files and volume for the db element but in the help message you are using the word media.

Personal taste

I would like the command append to be called attach instead, but this is personal preference and it's completely up to you :)

boyska · 2016-03-10T22:21:39Z

Ok, I fixed all your issues (I believe) and added some unit testing. It's not even near to be 100% complete, but I think it's not bad either.

adding documentation, asserts, exceptions

goodbye --mime

remember: calling sys.exit() is bad; raise some click Exception instead

libreant_db -> libreant-db media -> attachment

also, small fixes in documentation

insert-volume has a decent testing, export-volume, search and attach also slightly covered

additional infos can be found here: pallets/click#475

Upload file

the semantics needed to be explained better

Upload file

boyska mentioned this pull request Feb 5, 2016

Insert-volume/ feature in libreant-db cli #209

Closed

boyska force-pushed the upload-file branch 2 times, most recently from ed751ca to da9b14d Compare February 5, 2016 04:35

boyska force-pushed the upload-file branch from 0554197 to 53d02e8 Compare February 17, 2016 18:16

blallo and others added 10 commits March 10, 2016 17:23

Added first sketch of code to allow command line file upload

895e3c0

Working insert-volume subcommand in libreant-db

5242c97

Fixed the issue with single file attachment

bf816e0

refactor: map_attachments -> attach_list

ea74ca2

adding documentation, asserts, exceptions

mime can be automatically extracted

f8a809b

goodbye --mime

better help messages and error messages

6e9c0ad

documentation for insert-volume largely improved

e1f53fa

if mimetype can't be guessed, go on

17497c1

--name dropped, guessed as basename(filepath)

4b5c822

FIX error for files with no guessed mimetype

70a5216

boyska added 5 commits March 10, 2016 17:23

some (small) unit test about libreant-db

1fec7fb

more libreant-db tests

75c3676

remember: calling sys.exit() is bad; raise some click Exception instead

better wording

0ab21fb

libreant_db -> libreant-db media -> attachment

libreant-db append -> attach

4627d55

also, small fixes in documentation

Decent unit testing for libreant-db

8e88a27

insert-volume has a decent testing, export-volume, search and attach also slightly covered

boyska force-pushed the upload-file branch from d52c3dc to 8e88a27 Compare March 10, 2016 22:23

boyska added the cli label Mar 11, 2016

ael-code and others added 4 commits March 12, 2016 15:42

bugfix: BadOptionUsage accepts only one string on click==[6.0, 6.2]

a692970

additional infos can be found here: pallets/click#475

cli libreant-db: make METADATA optional

647bceb

Merge pull request #3 from ael-code/upload-file

41d51c6

Upload file

tests for insert-volume with no metadata

15c43f3

the semantics needed to be explained better

ael-code added a commit that referenced this pull request Mar 12, 2016

Merge pull request #242 from boyska/upload-file

7daf3a3

Upload file

ael-code merged commit 7daf3a3 into insomnia-lab:dev Mar 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload file #242

Upload file #242

boyska commented Feb 5, 2016

ael-code commented Feb 17, 2016

boyska commented Feb 17, 2016

ael-code commented Feb 28, 2016

boyska commented Feb 28, 2016

ael-code commented Feb 28, 2016

ael-code commented Feb 28, 2016

ael-code commented Feb 28, 2016

boyska commented Feb 28, 2016

boyska commented Mar 3, 2016

ael-code commented Mar 4, 2016

boyska commented Mar 4, 2016

ael-code commented Mar 5, 2016

boyska commented Mar 5, 2016

boyska commented Mar 10, 2016

ael-code commented Mar 10, 2016

boyska commented Mar 10, 2016

Upload file #242

Upload file #242

Conversation

boyska commented Feb 5, 2016

ael-code commented Feb 17, 2016

boyska commented Feb 17, 2016

ael-code commented Feb 28, 2016

boyska commented Feb 28, 2016

ael-code commented Feb 28, 2016

ael-code commented Feb 28, 2016

ael-code commented Feb 28, 2016

boyska commented Feb 28, 2016

boyska commented Mar 3, 2016

ael-code commented Mar 4, 2016

boyska commented Mar 4, 2016

ael-code commented Mar 5, 2016

boyska commented Mar 5, 2016

boyska commented Mar 10, 2016

ael-code commented Mar 10, 2016

Naming and typos

Personal taste

boyska commented Mar 10, 2016