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

Add a pyproject.toml #1710

Merged
merged 8 commits into from
Apr 19, 2023
Merged

Add a pyproject.toml #1710

merged 8 commits into from
Apr 19, 2023

Conversation

scarlehoff
Copy link
Member

@scarlehoff scarlehoff commented Apr 3, 2023

Any other options we should add?

(I guess eventually this file will contain the entire installation rules, for now I'd just like to add the isort and black options.
This is a first small step towards having pre-commit hooks*)

*which I hate

pyproject.toml Outdated Show resolved Hide resolved
scarlehoff and others added 2 commits April 3, 2023 14:34
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Co-authored-by: Roy Stegeman <roystegeman@live.nl>
@RoyStegeman RoyStegeman added the enhancement New feature or request label Apr 3, 2023
@Zaharid
Copy link
Contributor

Zaharid commented Apr 3, 2023

How is this supposed to be used? Is it going to be touching every file, or every edited file?

Can isort be made to only group imports in srdlib/external/internal, but not touch any other ordering?

Personally I stand by what I said here #1349 (comment) and more generally note that each of these things comes with tradeoffs costs that may be higher than telling every new contributor once "Please group imports in way".

@RoyStegeman
Copy link
Member

RoyStegeman commented Apr 3, 2023

Note that this does not automatically do anything - people still have to run isort and black manually. And as such I would only use it for touched files.

What this mainly does is provide standard settings for those tools such that people don't run these with different settings

@@ -0,0 +1,10 @@
[tool.black]
line-length = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add the option to not touch the string quotes (whatever the equivalent of let g:black_skip_string_normalization=1 is). This reduces unneeded disruption.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean ' to "?
But this I think we want for new code (as @RoyStegeman said, this is just the file that black and isort use for their config options when you run)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is likely enough that this gets executed in old files (where git add -uip would give you a point of pointless changes to sort trough) and also I don't particularly love the setting in new code, even if I'll use it if it's set already: it is the opposite choice of the Python repr, people do like to take issue with that https://github.com/grantjenks/blue, and personally I was used to https://stackoverflow.com/a/56190, from before people decided that having two string quotes was intolerable.

Copy link
Member Author

Choose a reason for hiding this comment

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

We started using black upon your suggestion.

That said, I don't have any strong opinion about style as long as it is consistent. Feel free to add any options to this .toml file.

Honestly, what would be great is to arrive to a set of options we are happy with and then run it for the entire codebase, skip the particular commit in the blame and continue from there.

Copy link
Member

Choose a reason for hiding this comment

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

Do those preferences really affect your efficiency though? I don't mean this to argue, but genuinely wondering.

If I had to do things manually I might decide to make it a bit more fancy as well (indeed for example https://stackoverflow.com/a/56190), but

  1. that doesn't really work with multiple people, unless we're going to put way more emphasis on formatting guidlines, so we'll have to end up accepting different formats anyway (if we don't use a tool, that is)
  2. More than getting to choose my own formatting I like not having to spend mental energy on it at all (within reason, e.g. pdfbases.py would probably require some care).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was expressing a desire but I realise it probably won't happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I am working on a personal project I enable the option to not touch the string marks. When I am working on a project that does change the quotes, then I do that. But that is not the main point I was making.

The argument is that for this project, in this context, it is better to have the option because it reduces the diffs when reformatting stuff, accidentally or otherwise, and because enabling the option is arguably of little value overall.

IMO the way to deal with string quotes in this project is not to add review comments expressing the desire to Marie Kondo the world in regards to string quotes (which is what has happened so far without particular issues).

Copy link
Member

@RoyStegeman RoyStegeman Apr 3, 2023

Choose a reason for hiding this comment

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

While I don't necessarily agree I would be happy to add skip-string-normalization = 1 (assuming that's correct) to the .toml file. The main benefit of black for me would be a universal way of enforcing line-length and indentation anyway, the quotes don't bother me as much.

Having said that - consistent quotes do spark joy :)

Copy link
Member

Choose a reason for hiding this comment

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

To come back to this, would both of you be ok with skip-string-normalization = 1? It still won't enforce anything but with this at least there would be consistency in output if we ask people to run black.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Co-authored-by: Roy Stegeman <roystegeman@live.nl>
Copy link
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

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

Ok for me this is fine.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@Zaharid Zaharid merged commit bd07c31 into master Apr 19, 2023
@Zaharid Zaharid deleted the scarlehoff-patch-1 branch April 19, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants