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

MAINT: Normalize code style of workflows.confounds #2729

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Mar 3, 2022

Changes proposed in this pull request

Documentation that should be reviewed

@effigies
Copy link
Member

effigies commented Mar 3, 2022

If this is just black, merge at will.

@effigies
Copy link
Member

effigies commented Mar 3, 2022

Or even just push the commit directly to master.

@oesteban
Copy link
Member Author

oesteban commented Mar 3, 2022

If this is just black, merge at will.

It's just black, but for some reason, it didn't normalize the single-quotes to double-quotes. So I did it manually and want to check I did not screw up :)

@effigies
Copy link
Member

effigies commented Mar 3, 2022

We disabled the quote normalization in config a while back.

@oesteban
Copy link
Member Author

oesteban commented Mar 3, 2022

We disabled the quote normalization in config a while back.

I've looked through setup.cfg in an attempt to find that but now I see it is in the pyproject.toml file.

Is there a particular reason for this? We have used both styles indistinctly, I would understand if we were following some convention...

@effigies
Copy link
Member

effigies commented Mar 3, 2022

The reason is mostly that black gets applied sometimes and it became hard to review PRs that mixed semantic changes and style fixes in unrelated portions of the code.

@oesteban
Copy link
Member Author

oesteban commented Mar 3, 2022

Makes sense. We could normalize though that a PR with only stylistic changes is submitted and the subsequent PR operates off of the styled branch. WDYT?

(that's actually what is going on in this PR, which I'm sending to then attack #2621 without style differences)

@effigies
Copy link
Member

effigies commented Mar 3, 2022

Yup, that's fine and what I'd prefer. Or even just separating the style commits.

@oesteban
Copy link
Member Author

oesteban commented Mar 3, 2022

@oesteban oesteban merged commit da24bf6 into master Mar 3, 2022
@oesteban oesteban deleted the sty/black-confounds branch March 3, 2022 21:43
@effigies effigies added this to the 22.0.0 milestone Mar 9, 2022
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