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

Subclass Assumptions Predicate from FOL #1

Open
wants to merge 2 commits into
base: FOL
Choose a base branch
from
Open

Conversation

sd-biswas
Copy link
Owner

  • Subclass assumptions.Predicate from FOL.Callable and rename it to AssumptionsPredicate.
  • Subclass assumptions.AppliedPredicate from FOL.Applied and rename it to AppliedAssumptionsPredicate.
  • Rename Function and AppliedFunction in FOL.

@sd-biswas sd-biswas mentioned this pull request Sep 19, 2014
11 tasks
@asmeurer
Copy link

Can you enable Travis CI on you fork so that the tests run here?

@sd-biswas
Copy link
Owner Author

Ask is completely screwed up at this moment. But before I get down to the messy business I want to confirm that this is exactly what you want, then I'll fix the issues.

@asmeurer
Copy link

I want you to enable Travis so I can see what is going wrong.


class Predicate(Boolean):
class AssumptionsPredicate(Callable):

Choose a reason for hiding this comment

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

I thought your class was called Predicate

Copy link
Owner Author

Choose a reason for hiding this comment

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

It is called Predicate. But the logic behind Predicate is in Callable and similarly logic behind AppliedPredicate is in Applied. FOL Predicate subclasses Callable and overrides the apply() method. Subclassing Predicate will produce the exact same result. So it is simply a question of preference.

Choose a reason for hiding this comment

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

I would subclass Predicate. That seems clearer, and assumedly its apply would be useful here.

@sd-biswas
Copy link
Owner Author

Yeah, I did. But I think it will probably start running after I make a commit (I'll dummy one now and later rebase it out).

@asmeurer
Copy link

This looks like it is the right idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants