-
Notifications
You must be signed in to change notification settings - Fork 5
Restructure database and stabilize back-end logic #93
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
Conversation
With the exception of engine/nodes/.
With the exception of E501. It's difficult to have 79 character lines while keeping docstring title lines on a single line.
tisnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, I've just dropped a couple of comments, but nothing that prevents the merge :)
| tuple[string] -- Tuple of local repository paths. | ||
| """ | ||
| if len(argv) == 1 or (len(argv) == 2 and argv[1] in ['-h', '--help', '--usage']): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you might start to accept, that the proposed argparse module might be better approach :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it most certainly is, especially once we begin to allow the user to specify options for the algorithms. I plan on switching to it as soon as I have some tests for the core/engine.
engine/results/detection_result.py
Outdated
| class DetectionResult: | ||
| """ | ||
| Represents the final result of a detection query. | ||
| Represent the final result of a detection query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably write:
Representation of the ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds awful, but I wanted to avoid having "of the"/"of a" twice in such a short sentence. The final result of a detection query. might work even better. Also, this change was an unfortunate and unintentional consequence of me using regular expressions to fix >500 docstyle violations.
engine/utils/list_tools.py
Outdated
| """ | ||
| flat = [] | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chain.from_iterable(listOfLists)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw itertools.chain.from_iterable() on StackOverflow, but I didn't want to add an import just for this, because the performance difference is negligible. Somehow I didn't realize itertools are in the standard library. I will change it within this PR. #95
The chain type doesn't allow many operations and it's not possible to repeatedly iterate over it.
NodeOriginclass) - this will be particularly useful later, when/if a code viewer is implementedurllib.parse.urlparse()instead of overly complicated read-only regular expressions (regexps are still used, but not for the whole URL, which is something of an improvement)requirements.txt=> close Add postgres package to requirements.txt #92errorspackage imports absolute => close Avoid relative imports by making subdirectory "errors" a module #82engine.utils.list_tools.flatten()withitertools.chain.from_iterable()=> close Replace function for list flatenning with standard library equivalent #95