juftin / camply

camply, the campsite finder ⛺️ - a tool to find campsites at sold out campgrounds through sites like recreation.gov
https://juftin.com/camply/
MIT License
464 stars 89 forks source link

Clarify weekends usage in example_search.yaml #331

Closed thornto4 closed 4 months ago

thornto4 commented 4 months ago

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:

thornto4 commented 4 months ago

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: https://github.com/juftin/camply/blob/d9f3e9a0761d9370841c1e64fcb373d3d4a72491/camply/cli.py#L371

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 commented 4 months ago

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: https://github.com/pallets/click/issues/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 commented 4 months ago

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 commented 3 months ago

:tada: This PR is included in version 0.32.3 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: