Skip to content

Improve suggestion for finding max and min using semver #337

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.

Sign up for GitHub

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

Jump to bottom
Merged
tomschr merged 2 commits into from
Apr 28, 2021
Merged

Improve suggestion for finding max and min using semver #337

tomschr merged 2 commits into from
Apr 28, 2021

Conversation

@mzjp2
Copy link
Contributor

@mzjp2 mzjp2 commented Apr 21, 2021

I was reading through the documentation and thought that the suggestion for finding max/min of a list/iterable of strings using semver unecessarily complicated, with a map, and multiple typecasts. I think using just max with key as done in this PR is much cleaner and should be the de-facto suggestion.

This is my first contribution to this repository, I hope it's welcome and I've followed the contribution guide well - but feel free to let me know if I can improve anything or close this PR if it's unwanted :)

@tomschr
Copy link
Member

tomschr commented Apr 21, 2021 •
edited
Loading

Thank you very much @mzjp2 and welcome! Much appreciated. 👍
You are right, your idea is much easier. I like it!

For some reason, there is an (unrelated) issue about the typing module. I'm not sure about the problem. @tlaferriere as you are more experienced than I am regarding typing, could you have a look, please?

After the problem is fixed, I'd like to merge it.

If you have further ideas, don't hesitate, just submit your suggestions. Thank you!

@tomschr tomschr added Doc Documentation related issue Enhancement Not a bug, but increases or improves in value, quality, desirability, or attractiveness labels Apr 22, 2021
@mzjp2
Copy link
Contributor Author

mzjp2 commented Apr 22, 2021

Thank you for the fix!

@tomschr
Copy link
Member

tomschr commented Apr 22, 2021

@mzjp2 @tlaferriere I've played a bit with the type definitions. For some reason, mypy seems to detect an error. I remember, this was different in the past, so they changed something. To avoid such thing in the future, maybe it would be useful to freeze the version.

I've introduced a new type DecoratorType and with that it seems to work. Maybe there is a better name.

Feel free to suggest a better name or handling.

@mzjp2
Copy link
Contributor Author

mzjp2 commented Apr 24, 2021

Sounds good to me - happy for you to merge whenever you want :)

@tomschr
Copy link
Member

tomschr commented Apr 26, 2021

Let's wait some more day, so Thomas has some time to look into it. 🙂

@tlaferriere
Copy link
Contributor

tlaferriere commented Apr 26, 2021

I'll look into this tomorrow, today I have an exam.

@tomschr
Copy link
Member

tomschr commented Apr 26, 2021

Good luck with the exam! Keep my fingers crossed. 🤞

@mzjp2
Copy link
Contributor Author

mzjp2 commented Apr 27, 2021 via email

tlaferriere
tlaferriere reviewed Apr 27, 2021
View reviewed changes
Copy link
Contributor

@tlaferriere tlaferriere left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Well spotted @mzjp2 this is much cleaner than before and very elegant.
@tomschr I haven't seen any mypy failure, I assume you fixed it? Anyway, apart from the suggestion, I approve this PR.

mzjp2 and others added 2 commits April 28, 2021 08:17
@mzjp2 @tomschr
Improve documentation for semver max/min; fix #337
f74e3a3
@tomschr @tlaferriere
Fix type definition problem
0398baa 
Define new type "Decorator" for function "deprecated" to
avoid this mypy error:

src/semver/_deprecated.py:69: error: Incompatible return value type
  (got "Callable[[VarArg(Any), KwArg(Any)], Callable[..., F]]",
   expected "Union[Callable[..., F], partial[Any]]")

Co-authored-by: Thomas Laferriere <tlaferriere@users.noreply.github.com>
@tomschr tomschr merged commit e93b6de into python-semver:master Apr 28, 2021
@tomschr
Copy link
Member

tomschr commented Apr 28, 2021

Thanks Thomas and Zain for your contribution, much appreciated! 👍 ❤️

@mzjp2 mzjp2 deleted the fix/doc-max-suggestion branch April 28, 2021 09:42
@mzjp2
Copy link
Contributor Author

mzjp2 commented Apr 28, 2021

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tlaferriere tlaferriere tlaferriere left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Doc Documentation related issue Enhancement Not a bug, but increases or improves in value, quality, desirability, or attractiveness

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mzjp2 @tomschr @tlaferriere