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.

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

list locations #235

Closed
wants to merge 0 commits into from
Closed

list locations #235

wants to merge 0 commits into from

Conversation

wgresshoff
Copy link
Contributor

@wgresshoff wgresshoff commented Jan 23, 2020

In order to set a default location I need to know which are existing.

Closes #234
Closes #232

Copy link
Member

@ppanero ppanero left a comment

Choose a reason for hiding this comment

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

Hi @wgresshoff Thanks a lot for this contribution, it is really helpful. At the moment the tests are not passing I think it is because the cli.py its lacking a last new line. Travis tests check for PEP-8 conventions, and one of those is that the python files should finish with a new line :)

I also see in the commit history that there is a "merge" commit. Could it be rebased? just so the history is clean.

In addition, the output does not show "nicely" in my (and probably also small terminals) due to the {:140}. Could it be shortened to maybe 20 or 30?

Thanks a lot!

@ppanero
Copy link
Member

ppanero commented Jan 24, 2020

As discussed IRL, the "set default" functionality will be generic so the create location will also make use of it when calling with --default

@with_appcontext
def default_location(name):
"""Set default location."""
from .models import Location
Copy link
Member

Choose a reason for hiding this comment

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

Imports should be on the beginning on the file.

@wgresshoff
Copy link
Contributor Author

Just added fix for #232

@ppanero
Copy link
Member

ppanero commented Jan 30, 2020

Slight refactoring to fixt tests. (@wgresshoff sorry I had to take out the pretty print it is a nice addition but it requires a bigger refactor along invenio).

Warning it is not backwards compatible:

  • CLI location command is now a group to contain its related commands create, list and set_default.
  • Location.get_by_name() returns now one_or_none instead of one and raising an exception if it does not exist.

@ppanero
Copy link
Member

ppanero commented Jan 30, 2020

Continued in #236

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli: list locations new default location should remove previous "default" ones.
2 participants