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

ci: add pep8-naming check to flake8 and ignore a bunch of warnings #425

Merged
merged 5 commits into from
Mar 26, 2021

Conversation

jenhagg
Copy link
Collaborator

@jenhagg jenhagg commented Mar 25, 2021

Purpose

Add pep8-naming check to flake8, so it is run by tox locally and in github.
See https://github.com/Breakthrough-Energy/RenewableEnergyProject/issues/343

What the code is doing

In this case, we ignore almost all the warnings. Note: adding the noqa on some lines also caused black to reformat into multi line declarations, which in turn requires moving the noqa back to the first line in that group. Hopefully we don't have to do this a lot.

Testing

tox -e flake8

Time estimate

5 min

@jenhagg jenhagg self-assigned this Mar 25, 2021
@jenhagg jenhagg added this to the Thunderstruck milestone Mar 25, 2021
Copy link
Collaborator

@BainanXia BainanXia 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. Thanks for taking care of this.

@danielolsen
Copy link
Contributor

@lanesmith can we come up with some more descriptive variable names for the cost curve visualizations?

@lanesmith
Copy link
Collaborator

@lanesmith can we come up with some more descriptive variable names for the cost curve visualizations?

@danielolsen, yeah certainly. What's the best way for me make those additions to this PR? Should I just checkout this branch?

@danielolsen
Copy link
Contributor

@danielolsen, yeah certainly. What's the best way for me make those additions to this PR? Should I just checkout this branch?

I suggest that you check out this branch, create a new branch of your own with your commit(s), and push that. Then @jon-hagg can cherry-pick your commits into this branch when ready, and there's no risk of conflicts.

@lanesmith
Copy link
Collaborator

@jon-hagg, I've finished updating the variable names for the cost curve visualization. The updates are in the branch lane/cost_curves_refactor. Let me know how you would like to proceed with this.

@jenhagg
Copy link
Collaborator Author

jenhagg commented Mar 26, 2021

@jon-hagg, I've finished updating the variable names for the cost curve visualization. The updates are in the branch lane/cost_curves_refactor. Let me know how you would like to proceed with this.

Thanks @lanesmith, probably the easiest way would be to create a PR to this branch, which (when merged) will effectively append your commits to the current PR.

Copy link
Contributor

@danielolsen danielolsen 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, thanks!

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