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

Base Discrete Maths Package #105

Closed
wants to merge 19 commits into from
Closed

Conversation

Retr05041
Copy link

Created a discrete maths cog and discrete maths package, along with a simple logical operations module for said package which includes:

  • Gates for: negation, conjunction, disjunction, implies, bi-implication, xor, nand, nor, xnor
  • Truth Table generator
  • Tautology checker

@Retr05041 Retr05041 changed the title ready for review Base Discrete Maths Package Jul 28, 2023
@PenguinBoi12 PenguinBoi12 added the ready to review Indicate that the PR is ready to be review by the team label Jul 28, 2023
@PenguinBoi12 PenguinBoi12 self-assigned this Jul 28, 2023
@PenguinBoi12 PenguinBoi12 self-requested a review July 28, 2023 20:33
Copy link
Contributor

@PenguinBoi12 PenguinBoi12 left a comment

Choose a reason for hiding this comment

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

I'm going to start by talking about the comments. Comments provide additional information that the code itself cannot convey. They should add context to what you're doing or explain complex parts of the code when necessary. Consequently, it's important to avoid redundant comments that merely restate what is already evident from the code's readability. Comments should be reserved for situations where the code is not self-explanatory.

Here's some example of some unnecessary comments.

# add a and b
def add(a, b):
    return a + b

# imports
from ... import ...
from ... import ...
import ....

# defines my_function
def my_function():
   ...

On another note, in Python you want to use doscstring when commenting functions, methods, classes or modules.

The second recurring issue I see is the use of abbreviation. Stuff like rpn, op, prop, etc. It's a bad habit. Abbreviation should be avoided as much as possible; there's only a few cases where they are accepted. Change them for full words.

As for the feature itself, it seems to work well. However, it would be nice to have a command to see how to use it. A sort of guide; we don't really know which symbols to use without looking at the code. Some of the errors we get are also not super interesting. For info I did a v v b
image
It is also strange that I can do stuff like a v b b or a a v b b , etc. I feel like it should be an error.

Aside from that well done. It looks like a great PR and everything seems to work well in general.

lib/parsers.py Outdated Show resolved Hide resolved
lib/parsers.py Outdated Show resolved Hide resolved
bot/extensions/discrete_maths_cog.py Outdated Show resolved Hide resolved
bot/extensions/discrete_maths_cog.py Outdated Show resolved Hide resolved
bot/extensions/discrete_maths_cog.py Outdated Show resolved Hide resolved
bot/extensions/discrete_maths_cog.py Outdated Show resolved Hide resolved
return all(results)

# Create a truth table from a proposition
def create_truth_table(proposition: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic that creates the table shouldn't be here. It should be in the cog. However, the logic that evaluates the logical proposition can/should stay here.

The reason why is that this library shouldn't care too much about how the evaluation of a proposition is used. It should only care about evaluating it and returning some results. Who cares about how it's used and what is displayed is the cog. The cog is the link between the logic and the user.

In other words, all the sanity checkers and logic for the evaluation is fine here but the code to draw (this includes making the variables) the table shouldn't be here but in the cog.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change the name for something else that represent a bit more clearly what it does (which is to evaluate logical proposition). Right now, logical_operation isn't very descriptive.

Also, it's in it's own package but it's alone? Wouldn't the parser module be included? Since this parser is built for this exactly that would be interesting to include it. Imagine if someone says "I want to take this libraries to use it somewhere else" but the parser is missing. It would be a bit awkward. So either move the parser there since it's for that only or move it somewhere else.

lib/discrete_maths/logic_operations.py Outdated Show resolved Hide resolved
lib/discrete_maths/logic_operations.py Outdated Show resolved Hide resolved
@PenguinBoi12 PenguinBoi12 added change needed Indicate that the PR need change before merging and removed ready to review Indicate that the PR is ready to be review by the team labels Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change needed Indicate that the PR need change before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants