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

Add '--continuous' check when '--notifications' enabled #93

Merged

Conversation

acaloiaro
Copy link
Contributor

If --notifications is provided without --continuous, it acts as a no-op, which can be confusing if you're testing notifications and don't realize that camply must be in continuous mode for notifications to be enabled.


Block on #92

camply/cli.py Outdated
if kwargs.get('notifications') and not kwargs.get('continuous'):
logger.error(
"To search with --notifications enabled, Camply must be run "
"in --continuous mode."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The concept of "continuous" mode isn't established elsewhere, is there a better way to phrase this?

camply/cli.py Outdated

if kwargs.get('notifications') and not kwargs.get('continuous'):
logger.error(
"To search with --notifications enabled, Camply must be run "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would "To search campgrounds with..." be clearer?

Should errors messages use the full parameter notation, i.e. --notifications, or simply parameter names, i.e notifications. I personally prefer the former, but want error messaging to be consistent throughout the app.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. I like something like: To receive notifications about campsites you must be searching continuously by passing the --continuous option.

Copy link
Owner

Choose a reason for hiding this comment

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

I like the mentioning of the full CLI arguments with the leading dashes

@juftin
Copy link
Owner

juftin commented Jul 4, 2022

Sorry to be a formatting bug here, would you make sure the code is being formatted with pre-commit as well? https://juftin.com/camply/contributing.html

Maybe I should put this info on the GitHub too?

@acaloiaro
Copy link
Contributor Author

Sorry to be a formatting bug here, would you make sure the code is being formatted with pre-commit as well? https://juftin.com/camply/contributing.html

Maybe I should put this info on the GitHub too?

Sure thing. Do you have any guidelines on when to add tests as well? This is a pretty simple PR, but it wouldn't be unreasonable to ask for additional test coverage over the new behavior.

@juftin
Copy link
Owner

juftin commented Jul 4, 2022

I'm not terribly worried about a test for this change here.

In general, camply has pretty poor test coverage. I have a number of command line tests that run inside the tox commandline environment but they don't really prove test cases beyond asserting that all run successfully.

I'd like to improve this in the future but I can't be a stickler for it in the meantime. I've implemented a lot more without a new test.

@acaloiaro acaloiaro force-pushed the add-continuous-check-when-notifications-enabled branch 2 times, most recently from 070356e to f839755 Compare July 4, 2022 21:45
@acaloiaro acaloiaro force-pushed the add-continuous-check-when-notifications-enabled branch from f839755 to fe403eb Compare July 4, 2022 22:11
Copy link
Owner

@juftin juftin left a comment

Choose a reason for hiding this comment

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

👍

@juftin juftin merged commit 98e5891 into juftin:main Jul 12, 2022
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.

None yet

2 participants