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 1 commit
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/
6 changes: 5 additions & 1 deletion pokedex/db/load.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']:
Copy link
Member

Choose a reason for hiding this comment

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

I think all and none can be handled in main.py, similar to how command_load does it:

pokedex/pokedex/main.py

Lines 232 to 235 in e5c18c4

if args.langs == 'none':
langs = []
elif args.langs is None:
langs = None

def include_row(row):
return languages[row.local_language_id].official
# If the none argument is passed then nothing should be changed from the csv files
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
11 changes: 10 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', 'all', or other languages like 'en,es' (default: all). The 'all' and 'none' codes cannot be used with other codes.")
Copy link
Member

Choose a reason for hiding this comment

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

The old help text seems sufficient to me (other than updating the default).

cmd_dump.add_argument(
'tables', nargs='*',
help="list of database tables to load (default: all)")
Expand Down Expand Up @@ -209,6 +209,15 @@ 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(',')]

# 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 progrm will close.
if 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