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

Add pythonic aliases for comparison predicates. #553

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

EricApostal
Copy link

[ ] Added changes to CHANGELOG.md
[ ] Bumped version number (delete if unneeded)

Changes proposed:
Add pythonic aliases for comparison predicates.
Ex: added are.greater_than as an alias of are.above

@davidwagner
Copy link
Member

Can you help me understand the rationale for why this addition is needed/beneficial?

@EricApostal
Copy link
Author

Can you help me understand the rationale for why this addition is needed/beneficial?

While keywords like above and below do make some sense, it's unconventional as comparison operators read like greater_than and less_than. It reads a lot more natural for those learning to code, as it keeps everything more in-line with coding convention.

@EricApostal
Copy link
Author

I would also like to add that this is the notation already used in the some of the function's descriptions.
Snippet from predicates.py:

@staticmethod
def above(y):
    """Greater than y."""

Copy link
Member

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

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

I can agree to seeing that this might be beneficial and it's great that it's just adding a simple alias, so there's no code maintainability issues. I just want to ensure that code testing coverage isn't hit as a result of this new code, so can we please add some testing code around the new methods?

Once we have the testing code, I can merge this in, it's in great condition. Thanks!

@EricApostal
Copy link
Author

I added methods replicating the existing ones. I've never worked with pytest, however, so please do let me know if anything needs to be amended.

@coveralls
Copy link

coveralls commented Jan 1, 2023

Coverage Status

Coverage: 92.689% (-0.05%) from 92.739% when pulling 4608d2f on SirTZN:master into 28b0841 on data-8:master.

Copy link
Member

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

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

Hey, this is very close to being mergable - thank you! You should be able to see updated coverage when you push new commits now.

@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.

This project adheres to [Semantic Versioning](http://semver.org/).

### v0.17.7
* Add pythonic aliases for comparison predicates.
* Add alias tests to test_predicates.py
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this line. Just the first one is fine :)


@staticmethod
def less_than_or_equal_to(y):
return are.below_or_equal_to(y)
Copy link
Member

Choose a reason for hiding this comment

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

Based on: https://coveralls.io/builds/55559799/source?filename=datascience%2Fpredicates.py

We're missing test coverage on this method and a few others. The tests you've created are fantastic so far, if you could please add a few more for the missing test methods - this should be mergeable!

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.

None yet

4 participants