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

Operation Node refactor #13

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

Conversation

radudum10
Copy link
Contributor

  • operation_node.py classes are now in separated files and some of them have been completely refactored.
  • I have fixed the dependencies between the classes.
  • This is currently still in progress, review and testing are welcome!

dnne and others added 13 commits May 31, 2019 16:47
Signed-off-by: Cristi Done <[email protected]>
Signed-off-by: Cristi Done <[email protected]>
Signed-off-by: Cristi Done <[email protected]>
… details in regex_parser_v3.py & sandbox_regex.py
Docstrings were added for all the functions. A bug caused by an unchecked pointer has been solved. Also, I have renamed functions, variables, extracted duplicate codes, made new functions in order to have a more readable code.
This is how the operation_node will be splitted in multiple files. Each of those files will contain extracted classes from thefile. Also, NonTerminalNode and TerminalNode classes have already been extractedand they are a good way to preview how all the files will look.
@radudum10 radudum10 added the WIP Work in progress label Aug 12, 2022
@radudum10 radudum10 requested a review from razvand August 12, 2022 19:03
@razvand
Copy link
Member

razvand commented Aug 28, 2022

@radudum10 , this requires rebasing.

This is still WIP so commits and commit messages are not that relevant now. But when getting closer to merging the PR, please be sure that:

  • commit messages are well written: see instruction here: https://chris.beams.io/posts/git-commit/
  • commit messages are complete and atomic: one commit is one functionality; there won't be a commit with two ideas inside it, or an idea spread across multiple commits

Copy link
Member

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Hi, @radudum10 . This is just a first pass. There's quite a lot of code to read, so I will need to take a deeper look. Please find some minor comments.

Also:

  • Please add an ending newline at each new file (see my comments).
  • Are submodules, and the .gitmodules file, required?
  • I would remove the .idea/ folder. This is mostly a configuration that should sit outside the project.


def is_allow_deny(self):
if self.match.is_terminal() and self.unmatch.is_terminal():
return self.match.terminal.is_allow() and self.unmatch.terminal.is_deny()
Copy link
Member

Choose a reason for hiding this comment

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

Add ending newline.

for node_iter in g.keys():
rv = rg.get_vertice_by_value(node_iter)
rg.reduce_graph_with_metanodes()
return rg
Copy link
Member

Choose a reason for hiding this comment

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

Add ending newline.

return result_str

def xml_str(self):
return self.recursive_xml_str(3, False)
Copy link
Member

Choose a reason for hiding this comment

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

Add ending newline.

return self.type == self.TERMINAL_NODE_TYPE_ALLOW

def is_deny(self):
return self.type == self.TERMINAL_NODE_TYPE_DENY
Copy link
Member

Choose a reason for hiding this comment

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

Add ending newline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants