Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pokedex dump -l argument error (#295) #299

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix pokedex dump -l argument error (#295) #299

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17f3624
Fix pokedex dump -l argument error (#295)
rluzuriaga Apr 1, 2020
8ad6443
Fix pokedex dump -l argument error - PR Changes
rluzuriaga Apr 3, 2020
cc3d5d7
Fix pokedex dump -l argument error - PR Changes
rluzuriaga Apr 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ install: pip install -e .
before_script: pokedex setup -v
script:
- py.test
- pokedex dump
- pokedex dump -l all
- git --no-pager diff --exit-code pokedex/data/csv/
4 changes: 4 additions & 0 deletions pokedex/db/load.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,10 @@ def dump(session, tables=[], directory=None, verbose=False, langs=None):
if langs is None:
def include_row(row):
return languages[row.local_language_id].official
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none should be handled in main.py too.

Copy link
Contributor Author

@rluzuriaga rluzuriaga Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@magical magical Apr 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

elif any(col.info.get('official') for col in table.columns):
def include_row(row):
return (languages[row.local_language_id].official or
Expand Down
15 changes: 14 additions & 1 deletion pokedex/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', or 'all' (default: all)")
cmd_dump.add_argument(
'tables', nargs='*',
help="list of database tables to load (default: all)")
Expand Down Expand Up @@ -209,6 +209,19 @@ def command_dump(parser, args):

if args.langs is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

langs = [l.strip() for l in args.langs.split(',')]

# If the langs code is only 'all' then langs is None so that all the tables get dumped.
if len(langs) == 1 and langs[0] == 'all':
langs = None

# Check if either 'all' or 'none' codes are used along side other codes.
# If either code is used, an error message will be displayed and the program will close.
elif len(langs) > 1 and 'all' in langs:
print("\nERROR: The 'all' code should be used by itself.")
return
elif len(langs) > 1 and 'none' in langs:
print("\nERROR: The 'none' code should be used by itself.")
return
else:
langs = None

Expand Down