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

Re organize code #126

Merged
merged 78 commits into from
Jun 6, 2018
Merged

Re organize code #126

merged 78 commits into from
Jun 6, 2018

Conversation

KevinHock
Copy link
Collaborator

@KevinHock KevinHock commented May 2, 2018

So I moved all of the files into their own directories, which was better than having almost all of them just in the pyt/ directory, I also made the tests nice and stuff

KevinHock and others added 30 commits April 20, 2018 17:35
@KevinHock
Copy link
Collaborator Author

Would you like to review the current state @omergunal?

self.blackbox_assignments = blackbox_assignments

def __repr__(self):
output = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Is self.nodes can be empty? If not, this is unnecessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need output to be defined already when we enter the for loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely :)

@@ -4,7 +4,8 @@
import ast


# Contains all project definitions for a program run:
# Contains all project definitions for a program run
# Only used in framework_adaptor.py, but modified here
project_definitions = dict()
Copy link
Contributor

@omergunal omergunal May 2, 2018

Choose a reason for hiding this comment

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

we can make a change on line 91 in this page like this:
Remove this:

if definition.node not in project_definitions:
      project_definitions[definition.node] = definition

and add this one

project_definitions.setdefault(definition.node, definition)

not obligatory but nicer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My only issue with that is that it has a return value that we don't use, I feel like it would be nicer if we were using the return value. I did enjoy using a defaultdict in definition_chains.py though.

Trims the reassigned list to just the vulnerability
chain.
-i, --interactive Will ask you about each blackbox function call in
vulnerability chains.
Copy link
Contributor

@omergunal omergunal May 2, 2018

Choose a reason for hiding this comment

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

Maybe you should add a link to last line for "How It Works?" A new user can find easily find pyt/pyt/README.rst

Copy link
Contributor

Choose a reason for hiding this comment

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

contribution link is also possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For this file I agree it needs more, was going to do this later on

Options
Configuration 
Links to other READMEs and docs

@KevinHock
Copy link
Collaborator Author

Still need to do

Options
Configuration

on the root README, but hoping merging this motivates me to do that and the vulnerabilities README

@KevinHock KevinHock merged commit 0246106 into master Jun 6, 2018
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.

4 participants