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

Jump to bottom

Cli setup #3

Merged
merged 1 commit into from Feb 24, 2023
Merged

Cli setup #3

merged 1 commit into from Feb 24, 2023

Conversation

adamshire123
Copy link
Contributor

What does this PR do?

app gets config settings from environment variables or defaults.
adds cli commands and options

Includes new or updated dependencies?

YES

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/IN-732

Developer

  • All new ENV is documented in README (or there is none)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

@adamshire123
Copy link
Contributor Author

I think something in cli.py is breaking pylama, but I'm not sure what.

@ehanson8
Copy link

It appears to be related to an incompatibility between pylama and pydocstyle due to some recent changes (link). We can remove pydocstyle from the pylama linters in setup.cfg as a quick fix but there's other linting error hiding behind that. And @hakbailey may have a better solution but it's annoying either way when we have to pin a version # or remember to add a linter back in.

Slightly unrelated, both Adam and I found out last week that running isort through pylama gives a very unhelpful message (Incorrectly sorted imports. [isort]) so we might consider restoring it as a separate linter in the template repo Makefile to get the useful messages back

@adamshire123 adamshire123 reopened this Feb 22, 2023
@adamshire123 adamshire123 reopened this Feb 22, 2023
hakbailey
hakbailey suggested changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

See inline comments for changes and suggestions.

setup.cfg Show resolved Hide resolved
sapinvoices/cli.py Outdated Show resolved Hide resolved
sapinvoices/config.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
sapinvoices/cli.py Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ehanson8
ehanson8 reviewed Feb 23, 2023
View reviewed changes
Copy link

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

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

Looks good to me but we can see if Helen has any other feedback

tests/conftest.py Show resolved Hide resolved
Makefile Show resolved Hide resolved
hakbailey
hakbailey reviewed Feb 23, 2023
View reviewed changes
Copy link
Contributor

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

This is ok as is but I think config could be simplified a bit (see inline comments).

Makefile Outdated Show resolved Hide resolved
sapinvoices/cli.py Outdated Show resolved Hide resolved
sapinvoices/config.py Outdated Show resolved Hide resolved
hakbailey
hakbailey approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

👍

@adamshire123
Set up initial CLI command and configuration
e7da799 
Why these changes are being introduced:
CLI command structure and config need to be updated from
the template repo defaults.

How this addresses that need:
* Adds expected command options and updates configuration as needed.
* Updates tests to reflect all changes.
* Updates README with descriptions for all required environemnt
variables

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/IN-732
@adamshire123 adamshire123 merged commit fe7f501 into main Feb 24, 2023
@adamshire123 adamshire123 deleted the cli-setup branch February 24, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehanson8 ehanson8 ehanson8 left review comments

@hakbailey hakbailey hakbailey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@adamshire123 @ehanson8 @hakbailey