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

Clarify weekends usage in example_search.yaml #331

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

thornto4
Copy link
Contributor

Description

Clarify comment for --weekends flag in example yaml config

The --weekends flag is intended to mean "weekends only" and will exclude weekdays from the search

Has This Been Tested?

No. But it's just a minor clarification to the documentation.

Checklist:

  • I've read the contributing guidelines of this project
  • I've installed and used .pre_commit on all my code
  • I have documented my code, particularly in hard-to-understand areas
  • I have made any necessary corresponding changes to the documentation

Clarify comment for --weekends flag in example yaml config

The --weekends flag is intended to mean "weekends only"
and will exclude weekdays from the search
@thornto4
Copy link
Contributor Author

thornto4 commented Mar 29, 2024

Hello! Thank you for such an amazing tool!

For some backstory, this PR is mostly a result of my inability to read :) So please feel free to close if you don't think it's helpful.

I was attempting to use the yaml config to configure my query for a few sites I was interested in. I started mostly by looking through the options in example_search.yaml and tweaking and tuning for my needs.

For whatever reason, when I got to weekends, I assumed it meant "include weekends too". Weekends at popular sites are certainly much harder to grab, so for whatever reason, I was assuming the defaults excluded these and the defaults would funnel you towards settings more likely to yield success.

Obviously that isn't the case.

The command line help (which I admittedly didn't read) makes this quite clear:

help="Only search for weekend bookings (Fri/Sat nights).",

So I figured it might be worth adding similar text to the comment in the example yaml to help avoid confusion for folks (like me) who assume intent instead taking the time to properly read the help documentation.

Anyways, thank you again!

@thornto4
Copy link
Contributor Author

thornto4 commented Mar 29, 2024

Another approach I considered, was seeing if you'd be willing to entertain a renaming of --weekends to --weekends-only.

Internally, it looks like we use weekends_only ... so that naming is at least more unambiguous and consistent with the internals.

But it doesn't look like Click currently has support for deprecating a parameter easily:
pallets/click#2263

However, they do have it added to a milestone for a future release. So if they add formal support for deprecating options and you'd be willing to entertain the change, I could certainly work to offer up that approach in a future PR.

@juftin
Copy link
Owner

juftin commented Apr 4, 2024

This looks great - thanks for the documentation improvement!

As far as changing --weekends to --weekends-only, I agree this makes the intention more clear - but I'd like to do everything I can to avoid breaking changes as the CLI API has been incredibly stable.

Introducing a command line alias could be kinda neat, I don't think I'd want to issue a deprecation warning though. An cool solution might be something like the below which would just clarify in the documentation that you can also use the --weekends-only alias too.

@click.option("--weekends", "--weekends-only", "weekends", is_flag=True, show_default=True, default=False)
def cli(weekends: bool) -> None:
    ...

Introducing too many options might be confusing as a user though and I don't want to get too fancy with Click so I'd probably lean towards leaving the CLI as is.

Again, thanks for the help here. This change will make things clearer for lots of people using YAML searches.

@juftin juftin merged commit 4cb9b1e into juftin:main Apr 4, 2024
1 check passed
@juftin
Copy link
Owner

juftin commented Apr 26, 2024

🎉 This PR is included in version 0.32.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants