Skip to content

Feature/support new search params #8

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

Merged
merged 77 commits into from
Mar 4, 2020

Conversation

antazoey
Copy link
Contributor

@antazoey antazoey commented Mar 2, 2020

@antazoey antazoey requested a review from alanag13 March 2, 2020 19:09
@alanag13
Copy link
Contributor

alanag13 commented Mar 4, 2020

I can still add a prompt if they don't use this password flag that just asks them, that may eliminate some of the UX concerns.

We should definitely do this either way for any time someone does profile set without the flag.
what's bothering me is that we need to have the flag in order to force the prompt to trigger again for the scenario where a user want to update their password.

maybe for profile set, we simply do what you just said (just prompt them whenever they use profile set, but we add a profile reset-pw subcommand. That would cover both cases. reset-pw would of course fail if you don't have a profile set yet.

@antazoey
Copy link
Contributor Author

antazoey commented Mar 4, 2020

I can still add a prompt if they don't use this password flag that just asks them, that may eliminate some of the UX concerns.

We should definitely do this either way for any time someone does profile set without the flag.
what's bothering me is that we need to have the flag in order to force the prompt to trigger again for the scenario where a user want to update their password.

maybe for profile set, we simply do what you just said (just prompt them whenever they use profile set, but we add a profile reset-pw subcommand. That would cover both cases. reset-pw would of course fail if you don't have a profile set yet.

Yes

@antazoey
Copy link
Contributor Author

antazoey commented Mar 4, 2020

Just made end_date go to 23:59:59 if you do not specify a time. This makes much more sense. If I give just a day, I'd expect to get all the events on that day. I am glad I caught this.



def _get_config_profile_from_parser(parser):
config_file_path = _get_config_file_path()
parser.read(config_file_path)
parser.read(config_file_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like a mistake (duplicate line)

README.md Outdated

To explicitly set your password, use `-p`:
(Optional) To set your password, use `--save-password`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're not using this anymore

_try_set_authority_url(args)
_try_set_username(args)
_try_set_ignore_ssl_errors(args)
_try_set_password(args)
config.mark_as_set_if_complete()
_ask_if_they_would_like_to_set_their_password()
Copy link
Contributor

Choose a reason for hiding this comment

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

_prompt_for_allow_password_set or something similar, for consistency

return True
def _ask_if_they_would_like_to_set_their_password():
answer = get_input(u"Would you like to set a password? (y/n): ")
if answer == u"y":
Copy link
Contributor

Choose a reason for hiding this comment

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

if I enter Y here, (capital y), it wont store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed + a test

@antazoey antazoey merged commit f3fb4a9 into master Mar 4, 2020
@antazoey antazoey deleted the feature/support-new-search-params branch March 16, 2020 17:02
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.

2 participants