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 Implication and Remove Nesting #27

Merged
merged 18 commits into from
Nov 23, 2021

Conversation

beckydvn
Copy link
Contributor

-Adds >> functionality to Var
-Creates an option to auto-simplify formulas upon creation by changing auto_simplify in the init module to True

@haz
Copy link
Collaborator

haz commented Sep 23, 2021

This looks pretty good to me! Can you throw in some tests here to make sure things work (mainly useful for future regressions). Be sure to test multiple children in the left/right formula (e.g., (x & y) & (w & z)).

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Nice additions!

Tox/flake8 has some complaints about formatting, can you try to fix those? CONTRIBUTING.md has instructions for running it but let me know if it's unclear.

After you fix the formatting it might complain about Mypy, I left tips for that in the review.

@beckydvn beckydvn requested a review from blyxxyz September 28, 2021 22:30
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Great! The test passes, and mypy passes, so this is almost done.

flake8 still has a few complaints:

nnf/__init__.py:17:1: F401 '_typeshed.IdentityFunction' imported but unused
nnf/__init__.py:87:1: E305 expected 2 blank lines after class or function definition, found 1
nnf/__init__.py:118:39: E225 missing whitespace around operator
nnf/__init__.py:1741:1: E305 expected 2 blank lines after class or function definition, found 1

Tox should also list those problems, have you managed to get it set up?

(We should probably add CI so Github runs the checks automatically when you open a PR, that makes things easier.)

beckydvn and others added 2 commits October 25, 2021 09:32
Co-authored-by: Jan Verbeek <jan.verbeek@posteo.nl>
@beckydvn beckydvn requested a review from blyxxyz October 25, 2021 13:40
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for bearing with me.

@beckydvn
Copy link
Contributor Author

beckydvn commented Nov 4, 2021

No worries, thanks for taking a look! Hope it proves to be useful in the long run :)

@haz haz merged commit c2e81fd into QuMuLab:master Nov 23, 2021
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