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

Predicates (and fix minimize so that tests pass) #209

Merged
merged 13 commits into from
Mar 23, 2016
Merged

Predicates (and fix minimize so that tests pass) #209

merged 13 commits into from
Mar 23, 2016

Conversation

papajohn
Copy link
Contributor

@papajohn papajohn commented Mar 5, 2016

No description provided.

@papajohn
Copy link
Contributor Author

papajohn commented Mar 5, 2016

Implements #207

@@ -3,4 +3,5 @@
from .tables import *
from .formats import *
from .maps import *
from .predicates import *
Copy link
Member

Choose a reason for hiding this comment

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

This means that all the functions in predicates.py will be in the global scope when we run from datascience import *.

Are we concerned about potential name conflicts since predicates.py has a lot of functions?

@SamLau95
Copy link
Member

SamLau95 commented Mar 6, 2016

Looks mostly good to me, aside from a question and a suggestion.

We should also add predicates.py to the docs by adding a line in docs/index.rst and creating a predicates.rst file that looks like the util.rst file.

@papajohn
Copy link
Contributor Author

papajohn commented Mar 6, 2016

I suppose we could put all predicate functions as attributes of some
class... I'll think about it. Something like,

t.where('age', Item.between(3, 8))

On Sun, Mar 6, 2016 at 2:55 AM, Sam Lau notifications@github.com wrote:

Looks mostly good to me, aside from a question and a suggestion.

We should also add predicates.py to the docs by adding a line in
docs/index.rst and creating a predicates.rst file that looks like the
util.rst file.


Reply to this email directly or view it on GitHub
#209 (comment).

@peterasujan
Copy link
Contributor

If you end up pursuing this method, I might suggest using "is" (I guess it should be capitalized though) as the class name, so that the whole line reads (almost) like English.

@papajohn
Copy link
Contributor Author

papajohn commented Mar 8, 2016

Ok, all single predicates are now methods of are. (I considered Is, but requires caps to avoid keyword and ls and Is look the same.)

I left both and either in the global namespace as well as in are because it looks prettier...

t.where('age', either(are.equal_to(5), are.above(8))

vs

t.where('age', are.either(are.equal_to(5), are.above(8))

@papajohn papajohn changed the title Predicates Predicates (and fix minimize so that tests pass) Mar 23, 2016
@papajohn papajohn merged commit 520172c into master Mar 23, 2016
@papajohn papajohn deleted the pred branch March 23, 2016 02:39
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