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

chore : Removed vestigial/no longer needed pylint disable directives #1327

Merged
merged 6 commits into from
Aug 11, 2021

Conversation

GurpreetSarangal
Copy link
Contributor

@GurpreetSarangal GurpreetSarangal commented Aug 6, 2021

fix #1247

Copy link
Contributor

@BreadGenie BreadGenie left a comment

Choose a reason for hiding this comment

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

Looks good!

One thing to note is that we are following https://www.conventionalcommits.org/ so try to follow that in future PRs (PR title and commit messages) :D

Also you can link a PR to an issue using Fix <issue-number> so here you can edit the PR description to Fix #1247 to link it.

@Molkree
Copy link
Contributor

Molkree commented Aug 6, 2021

More on linking PR to the issue, you can use lots of words for that.

@GurpreetSarangal GurpreetSarangal changed the title Removed vestigial/no longer needed pylint disable directives Fix #1247 Removed vestigial/no longer needed pylint disable directives Aug 6, 2021
@GurpreetSarangal GurpreetSarangal changed the title Fix #1247 Removed vestigial/no longer needed pylint disable directives Fix #1247: Removed vestigial/no longer needed pylint disable directives Aug 6, 2021
@GurpreetSarangal
Copy link
Contributor Author

I didn't know that. Thanks !

@BreadGenie
Copy link
Contributor

BreadGenie commented Aug 7, 2021

@GurpreetSarangal the PR title doesn't need to mention an issue. You can replace the Fix #1247 (in the title) to something like chore (one of the types mentioned in conventional commits). You can take a look at other PRs to get a feeling of how conventional commits works.

BTW you can join our gitter chat where we discuss about the tool and help each other. :D

@GurpreetSarangal GurpreetSarangal changed the title Fix #1247: Removed vestigial/no longer needed pylint disable directives chore : Removed vestigial/no longer needed pylint disable directives Aug 7, 2021
@GurpreetSarangal
Copy link
Contributor Author

GurpreetSarangal commented Aug 7, 2021

Is it okay now? or I have to make changes in commit message also?

@BreadGenie
Copy link
Contributor

Yep, I think it's okay now.

If you really wanna change the commit message you can

  • git commit --amend in the feature branch

  • change the commit message

  • git push origin +<your-feature-branch>

@GurpreetSarangal
Copy link
Contributor Author

Thanks for your guidance. Can you review it again?

@BreadGenie
Copy link
Contributor

It seems like there's some merging happened from my_branch to my_branch, but the other commit message looks fine. Don't worry about the merge commit, while merging this PR maintainers will be able to clean up the commit messages.

@GurpreetSarangal
Copy link
Contributor Author

I think I have made another branch of name 'my_branch' locally from the forked repository.

@BreadGenie
Copy link
Contributor

Oops I should've explained better. By feature branch what I meant was the current branch. But as long as you learned something it's all good :D

@GurpreetSarangal
Copy link
Contributor Author

Yeah, I've learned a new git command from you.

@terriko
Copy link
Contributor

terriko commented Aug 9, 2021

Looking good! I've started github actions on this so it should be running tests now. (due to github suffering some abuse, they ask maintainers to ok the first run from all new contributors; it should run for you automatically after this!)

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2021

Codecov Report

Merging #1327 (4a4d38b) into main (9159639) will increase coverage by 1.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
+ Coverage   79.08%   80.46%   +1.38%     
==========================================
  Files         271      271              
  Lines        4881     4879       -2     
  Branches      585      586       +1     
==========================================
+ Hits         3860     3926      +66     
+ Misses        872      810      -62     
+ Partials      149      143       -6     
Flag Coverage Δ
longtests 80.46% <100.00%> (+1.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cve_bin_tool/async_utils.py 91.66% <ø> (ø)
cve_bin_tool/checkers/expat.py 100.00% <ø> (ø)
cve_bin_tool/checkers/libgcrypt.py 100.00% <ø> (ø)
cve_bin_tool/checkers/libjpeg_turbo.py 100.00% <ø> (ø)
cve_bin_tool/checkers/liblas.py 100.00% <ø> (ø)
cve_bin_tool/checkers/systemd.py 100.00% <ø> (ø)
cve_bin_tool/checkers/xerces.py 100.00% <ø> (ø)
cve_bin_tool/cli.py 78.31% <ø> (+3.01%) ⬆️
cve_bin_tool/extractor.py 57.86% <ø> (ø)
cve_bin_tool/util.py 73.26% <ø> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91041e9...4a4d38b. Read the comment docs.

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Looks like black is happy, so we should be good to merge this. Thank you!

@terriko terriko merged commit a20c29b into intel:main Aug 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.

Remove vestigial/no longer needed pylint disable directives
5 participants