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

changed wid to omega_ideal #2143

Closed
wants to merge 4 commits into from
Closed

changed wid to omega_ideal #2143

wants to merge 4 commits into from

Conversation

kurt-rhee
Copy link
Contributor

  • Closes What does "wid" stand for? #2137
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

@cwhanse
Copy link
Member

cwhanse commented Jul 25, 2024

@pvlib/pvlib-maintainer we haven't adopted black as the formatter. When PRs use black to fix lint errors, it appears that black also reformats lines that the submitter doesn't change. Are we OK with this? I think it's a minor concern, unless, clutter in the blame history is not a minor concern.

@AdamRJensen
Copy link
Member

@pvlib/pvlib-maintainer we haven't adopted black as the formatter. When PRs use black to fix lint errors, it appears that black also reformats lines that the submitter doesn't change. Are we OK with this? I think it's a minor concern, unless, clutter in the blame history is not a minor concern.

My concern is mainly that it makes reviewing more difficult. If we want to apply black then we should do the whole package in one go. I'm -1 on individual PRs doing it.

@echedey-ls
Copy link
Contributor

Given that @kurt-rhee is a big fan of VSCode and Rust, I'm assuming he is using ruff, right? For context, it is a Python linter and formatter written in Rust. And it has a plugin for VSCode.

Kurt, in case you didn't know, you can format only some selected lines by selecting them and right-clicking, then click on Format Selection

image

This is the real behind-the-scenes of pretty much all my PRs

@kurt-rhee kurt-rhee closed this by deleting the head repository Jul 26, 2024
@kurt-rhee
Copy link
Contributor Author

@echedey-ls I was using black just to be lazy and not format the flake8 line lengths manually. I'll try out Ruff next, the format selection feature looks really nice!

New PR here without any autoformatting: #2146

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