Fix pokedex dump -l argument error (#295) #299
Conversation
pokedex/main.py -
create_parser() -
Change the help message for the langs argument in the dump subparser
to show the actual default and state that the 'all' and 'none' codes
cannot be used alongside other codes.
command_dump() -
Check if 'all' or 'none' codes are passed alongside other codes. If
they are, error message is printed and program ends.
pokedex/db/load.py -
dump() -
Add check if langs code is 'all' or 'none'.
If langs wasn't passed to the parser or 'all' was passed (they are
the same since the default is 'all'), then everything will get
dumped to the csv files.
If 'none' was passed to the parser, then nothing new should be
dumped to the csv files.
pokexed/.travis.yml -
Re-added 'pokedex dump -l all' that was previously remove on
77e3d9d
Resolves: veekun#295
pokedex/db/load.py
Outdated
| @@ -431,9 +431,13 @@ def dump(session, tables=[], directory=None, verbose=False, langs=None): | |||
| # if specified, or for official languages by default. | |||
| # For non-translation tables, dump all rows. | |||
| if 'local_language_id' in columns: | |||
| if langs is None: | |||
| # If no lang arguments were passed or the 'all' argument was passed | |||
| if langs is None or langs == ['all']: | |||
There was a problem hiding this comment.
I think all and none can be handled in main.py, similar to how command_load does it:
Lines 232 to 235 in e5c18c4
pokedex/main.py
Outdated
| @@ -114,7 +114,7 @@ def create_parser(): | |||
| help="directory to place the dumped CSV files") | |||
| cmd_dump.add_argument( | |||
| '-l', '--langs', dest='langs', default=None, | |||
| help="comma-separated list of language codes to load, 'none', or 'all' (default: en)") | |||
| help=u"comma-separated list of language codes to load, 'none', 'all', or other languages like 'en,es' (default: all). The 'all' and 'none' codes cannot be used with other codes.") | |||
There was a problem hiding this comment.
The old help text seems sufficient to me (other than updating the default).
All changes requested in PR 17f3624 ### pokedex/main.py - #### create_parser() - - Change to langs argument reverting the help message to the original one but changing the default to 'all'. #### command_dump() - - Add check for 'all' langs code instead of in the load.py file. - Add/fix comments. --- ### pokedex/db/load.py - #### dump() - - Remove unneeded check for 'all' langs code since not it is checked in the main.py file. - Reword comment of what happens when the 'none' code is passed.
pokedex/db/load.py
Outdated
| # If the none code is passed, then all the csv files with the local_language_id | ||
| # column are not updated. In other words they are left blank. | ||
| elif langs == ['none']: | ||
| return False |
There was a problem hiding this comment.
none should be handled in main.py too.
There was a problem hiding this comment.
I don't thing that can be handled in main.
If the user enters pokedex dump -l none then the parser has to pass none as the code to load.py. A code of none is not the same as the parser not getting any code. If the parser doesn't get any code, then pokedex dump langs defaults to all.
The load.py file is the one that figures out what to dump to the csv files depending on the langs code(s). So it checks if no codes are passed (pokedex dump OR pokedex dump -l all since they are treated the same) and dumps everything to the csvs. If pokedex dump -l none is entered then the return False from line 440 makes the function only dump the tables that do not contain the local_language_id column identifier.
I hope I made this at least somewhat clear, its a bit complicated to explain over plain text.
EDIT: fixed typos
There was a problem hiding this comment.
Oh, I missed that detail. You're right that the change i suggested isn't equivalent to the PR's code; however, i don't think that PR's behaviour is correct. The comment in the dump function makes it clear that the langs argument is only supposed to apply to unofficial translations; official text is always loaded, regardless of language. I think the CLI should preserve that behaviour - dump -l none should only disable dumping of translations, not all text. That's consistent with how load -l works too.
Edit: there's a bug too. Returning here will abort the entire dump as soon as any translation table is encountered. To get the behaviour you described, i think you would want a continue.
| @@ -209,6 +209,19 @@ def command_dump(parser, args): | |||
|
|
|||
| if args.langs is not None: | |||
There was a problem hiding this comment.
Something like this:
if args.langs == 'none':
langs = []
elif args.langs is None or args.langs == 'all':
langs = None
else:
langs = [l.strip() for l in args.langs.split(',')]
# etc...There was a problem hiding this comment.
If I do something like this then in load.py I would have to change
elif langs == ['none']:
return False
to
elif len(langs) == 0:
return False
So I would think that its pretty much the same thing
### pokedex/main.py - #### create_parser() - - Change the default in the the help text for the `-l` argument in the `pokedex dump` parser. #### command_dump() - - Change the functionality of `pokedex dump -l none` to be the same as entering `pokedex dump`. --- ### pokedex/db/load.py - #### dump() - - Change the functionality of the dump command to work the way that the help text says it should. - Tables always dump official languages. - If there were any `langs` passed, then the official languages plus the specified `langs` will be dumped from the tables that have a 'local_language_id' column. - If `pokedex dump -l all` is passed then all the languages (official and unofficial) will be dumped. - If the table doesn't have a 'local_language_id' column, then all the rows will be dumped.
|
@magical Any update on this PR? I know it is failing the Travis CI build but that's only because the original dump command was giving the wrong result (which is what this PR fixes) so the |
|
@rluzuriaga Sorry. Thanks for the reminder. I'll take a look. It seems to be failing because it is dumping the Czech translations where it wasn't before. Hmm. I do still want to test |
pokedex/main.py -
create_parser() -
to show the actual default and state that the 'all' and 'none' codes
cannot be used alongside other codes.
command_dump() -
they are, error message is printed and program ends.
pokedex/db/load.py -
dump() -
the same since the default is 'all'), then everything will get
dumped to the csv files.
dumped to the csv files.
pokexed/.travis.yml -
77e3d9d
Resolves: #295