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

Deprecating Choices.default #139

Merged
merged 4 commits into from
Feb 27, 2024
Merged

Conversation

HalfWhitt
Copy link
Contributor

@HalfWhitt HalfWhitt commented Feb 23, 2024

I was poking around and noticed that setting default on a Choice seems to not do anything. It's only ever referenced again when validating, and then only does anything if the value being validated is None, but a choice is never asked to validate None. Removing this code has no effect on any of the tests.

Is this just a remnant from before #33? If so, I can add a deprecation warning, remove its use from Travertino's tests (and add one to check it's deprecated), and also remove it from a handful of choices defined in Toga.

PR Checklist:

  • All new features have been tested
  • I have read the CONTRIBUTING.md file
  • All new features have been documented
  • I will abide by the code of conduct

@HalfWhitt HalfWhitt changed the title Removed internal usage of choice.default, which had no effect Deprecating Choice.default Feb 23, 2024
@HalfWhitt
Copy link
Contributor Author

And I guess the caveat is that if it is there for a reason that's not immediately apparent, should it be doing something?

@freakboy3742
Copy link
Member

Jeez, whoever wrote this really should have written some documentation... (checks blame log)... oh :-)

I think your analysis is correct - this is an artefact of the change introduced by #33. With that behavior change, default seems safe to deprecate.

The two things to check for confirmation will be the Toga test suite, and the Colosseum test suite; although a bug on the latter isn't necessarily a dealbreaker, as Colosseum hasn't seen a whole lot of development.

@HalfWhitt
Copy link
Contributor Author

Confirmed that Toga test suite runs fine with the dangling code removed. I tried testing Colosseum, but it neither depends on Travertino nor has quite the same relevant bits in its analagous code, so it's apples and oranges.

@freakboy3742
Copy link
Member

Confirmed that Toga test suite runs fine with the dangling code removed. I tried testing Colosseum, but it neither depends on Travertino nor has quite the same relevant bits in its analagous code, so it's apples and oranges.

Huh... Colosseum is lagging further behind than I remembered...

@rmartin16
Copy link
Member

@HalfWhitt just in case it hasn't been mentioned before, if you run pre-commit install, the pre-commit checks that run in CI will automatically run anytime you try to commit locally.

@HalfWhitt
Copy link
Contributor Author

Thanks @rmartin16. I hadn't done it at first (didn't think about Travertino being a separate repository from Toga), and then after I added it, it wasn't seeing the changenote file since it wasn't part of the newer commit. All sorted now : )

@HalfWhitt HalfWhitt changed the title Deprecating Choice.default Deprecating Choices.default Feb 26, 2024
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation and fix!

@freakboy3742 freakboy3742 merged commit 7672a97 into beeware:main Feb 27, 2024
8 checks passed
@HalfWhitt HalfWhitt deleted the deprecate-default branch February 27, 2024 22:57
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.

3 participants