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

feat: added prospector code analysis tool #688

Closed
wants to merge 9 commits into from
Closed

feat: added prospector code analysis tool #688

wants to merge 9 commits into from

Conversation

vickywane
Copy link

This pull request will fix this issue.

This pull request adds Prospector as a static code analysis tool to run an analysis each time a PR is opened through the use of GitHub actions.

@@ -0,0 +1 @@
prospector==1.5.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Alright.

That works.

.prospector.yml Outdated
Comment on lines 11 to 18
max-line-length: 120

pep8:
full: true
disable:
- N803 # argument name should be lowercase
- N806 # variable in function should be lowercase
- N812 # lowercase imported as non lowercase
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you source these from the .flake8 file? Alt jut disable this check since we already run falke8 automatically to release

Copy link
Author

Choose a reason for hiding this comment

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

Alright.

Copy link
Author

Choose a reason for hiding this comment

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

I have disabled the checks.

I think they can be added later.

@George3d6
Copy link
Contributor

Afaict the analysis finds nothing:

         Started: 2021-10-25 22:30:27.680064
        Finished: 2021-10-25 22:30:27.732671
      Time Taken: 0.05 seconds
       Formatter: grouped
        Profiles: default, no_doc_warnings, no_test_warnings, strictness_medium, strictness_high, strictness_veryhigh, no_member_warnings
      Strictness: None
  Libraries Used: 
       Tools Run: dodgy, mccabe, pep8, profile-validator, pyflakes, pylint
  Messages Found: 0

Which is kind of strange... as in, I'm worried it might not be analyzing all the codebase. Can you intentionally add an error somewhere to double check that it works?

@vickywane
Copy link
Author

Which is kind of strange... as in, I'm worried it might not be analyzing all the codebase. Can you intentionally add an error somewhere to double check that it works?

Quite strange. Locally, i can see output from the run. See screenshot ;

Screenshot_20211027_153902

@vickywane
Copy link
Author

@George3d6 I just refactored the prospector to save the output in a JSON and upload it to GitHub artifiacts. You can view it in the artifacts tab for a CI run.

Are you okay with this approach?

@George3d6
Copy link
Contributor

I think this approach work, but could you link to the artefact generated by this execution? -- currently I don't see anything: https://github.com/mindsdb/lightwood/runs/4023079796?check_suite_focus=true

@George3d6
Copy link
Contributor

Any updates on this? I feel like there may have been a few merging issues along the way :))

Feel free to open a new PR if you want.

@vickywane
Copy link
Author

I’ll pull the new merges into my branch.

The underlying problem is that the Github CI checks are not printing out the analysis result to the console, making it difficult to work with.

@George3d6
Copy link
Contributor

Closing as inactive for now, might be better to just start a new PR anyway, since this branch resulted in a very odd diff.

@George3d6 George3d6 closed this Nov 11, 2021
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.

2 participants