Skip to content

Part 2/3 of Cookiecutter pre-commit workflow (flake8) #54

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

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

bobleesj
Copy link
Contributor

@bobleesj bobleesj commented Aug 2, 2024

As instructed in #53 (review), here, I created a branch called flake8 from the upstream cookie branch.

Changed:

Just to confirm my workflow:

# Add upstream
git remote add upstream https://github.com/diffpy/diffpy.snmf

# Fetch all branches and updates from the upstream repository
git fetch upstream 

# Creates a local branch named cookie that tracks upstream/cookie
git checkout -b cookie upstream/cookie

# Ensure the latest updates
git pull

# Make a local branch from cookie
git checkout -b flake8

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

great work, please see inline comment.

# normalized_grid / stretching_factor, normalized_grid, self.iq, left=0, right=0
# )

# E731 do not assign a lambda expression, use a def
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment. Less chatter is better in the future, though I appreciate you wanting to communicate with me what you are doing (I think I can figure it out though.

For these lambda flake8 errors we would normally see if the function is being used in more than one place, and if it is, define a private function somewhere outside the main part of the code. If it is only being used in one place, we would simply move the lambda to where it is being used (so not assign it to a variable and then put the variable where it is being used, or we could put a # noqa: E731 at the end of this line so we leave it as is but it won't fail flake8.

How do we decide? the objective of all of this is not to pass flake8 but to make the code better, so the choice we make is the one that will make the code the best.

Also, in general, don't leave commented code in there, just delete it. Again, I appreciate the impulse (more transparency) but we don't want to carry around every old version of code in hte future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. A new pull request addressing your feedback has been made: #56

@sbillinge
Copy link
Contributor

As instructed in #53 (review), here, I created a branch called flake8 from the upstream cookie branch.

Changed:

* Fixed line-width of 115 error (E501)

* Removed unused imports of functions and packages

* CI passes (https://github.com/bobleesj/diffpy.snmf/actions/runs/10208965578)

Just to confirm my workflow:

# Add upstream
git remote add upstream https://github.com/diffpy/diffpy.snmf

# Fetch all branches and updates from the upstream repository
git fetch upstream 

# Creates a local branch named cookie that tracks upstream/cookie
git checkout -b cookie upstream/cookie

# Ensure the latest updates
git pull

# Make a local branch from cookie
git checkout -b flake8

this workflow is spot on. I think you know, but just to say, you only have to connect to upstream and fetch once-and-done. Once you fetch, you should be able to just git checkout cookie and it should already be set to track upstream/cookie, but doing it your way is more explicit. Again, this is once and done, once you set cookie to track upstream/cookie, afte rthat you just need to do git pull to sync . Nice work.

@sbillinge sbillinge merged commit 90dd060 into diffpy:cookie Aug 2, 2024
1 of 2 checks passed
@bobleesj bobleesj deleted the flake8 branch August 2, 2024 15:25
@bobleesj
Copy link
Contributor Author

bobleesj commented Aug 2, 2024

git checkout cookie

-> Switched to branch 'cookie'
-> Your branch is up to date with 'upstream/cookie'.

Once-and-done, confirmed! @sbillinge

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