-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Flake8, black, new API endpoints #89
Conversation
1a20f27
to
65b1acc
Compare
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.
Thank you for the PR.
I have just one question. I noticed that you explained why you did not use black in the analyzers code. Did you try to apply it and you found some problems? I understand that it could bring some issues but I think that the testing suite can help us to avoid errors. Can you give it a chance by changing these files too and running the tests locally? In this way the code will be uniform everywhere and we can force little contributors to follow style rules (writing a new analyzer is fast and easy. It is difficult that the main parts of the application will be changed by external people while analyzers are a good target for first contributions). What do you think about?
Thanks for the quick review. I did try to use black for the analyzers code, the problem is not with just black but the fact that we have to make 3 new changes in each analyzer file, sequentially: replace all Maybe we could just enforce black for now and leave f-strings and flake8 issues (only if there are too many of them), but then later when we do switch to f-strings we would need to enforce black again. |
Example:
This is also because black isn't always able to restrict the lines to 88 characters. (related GitHub issue) |
If you did not see critical problems after this when running tests, we can do it I think. |
By convention in python 3.x, a regex string should be prepended with So, after making the requested changes, should I squash my new commits into the previous commit only or should I make these changes in a new commit ? |
…points, cors handling, better URL schemas
65b1acc
to
1c4eacd
Compare
for me this is ok, thank you |
Major Changes:
api_app/views.py
has been deleted in favor of,api_app/gui.py
which includes the routes used by the django templates and,api_app/api.py
which includes the API endpoints used by the official pyintelowl CLI client and the official IntelOwl-ng web client./api/
) fromintel_owl/urls.py
are now in the newapi_app/urls.py
and referenced in the former. This IMHO increases modularity and code maintainability. More info here.query_database_json
has been removed since we are now usingJobViewSet
API viewclass feature by Django-rest-framework which provides us automatic routing and serialization for doing operations on the Job Model. More info here. I've updated the href link in thetemplates/query_database.html
to use this new route.TagViewSet
API class inapi_app/api.py
to provide automatic routing and CRUD operations on Tags Model. (This is used by the IntelOwl-ng client).obtain_user_token
,get_user_info
andperform_logout
APIs which closes Authentication Service (DRF) #80. These routes are for authenticating and de-authenticating user and their tokens.Minor Changes:
api_app/utilities.py
(diff).api_app/script_analyzers
directory) are updated to replacestr.format()
withf-strings
format. (f-strings everywhere #83)Changes related to linting:
requirements.txt
.travis.yml
,code-style: black
badge to README.mdapi_app/script_analyzers
directory. The reason for this being, though we can directly run black and it will lint those files, black can sometimes write breaking changes so we should test each file one by one to be on the safer side.TESTS: