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

fix #329: fmt.diff checks same paths as ci._check_files #332

Merged
merged 7 commits into from
Dec 27, 2022

Conversation

rorour
Copy link
Contributor

@rorour rorour commented Dec 23, 2022

The problem I mentioned in today's meeting was in fact a bug that was fixed in version 22.12.0, and we only required >=22.0 (I was running 22.6.0.)

This update is enforcing new requirements, hence the change to pykern/pkcli/github.py and pykern/xlsx.py.

rshellweg passes ci format checks on my machine with these changes, but running pykern fmt run . will change files that pykern ci run does not diff. (Running pykern fmt run . in rshellweg will format those example files that were failing pykern ci run). Do we want pykern fmt run . to only run on <pkg>, tests, setup.py when run from the repo root?

@rorour rorour requested a review from robnagler December 23, 2022 02:36
@robnagler
Copy link
Member

Do we want pykern fmt run . to only run on , tests, setup.py when run from the repo root?

Correct. We don't want to enforce standards on non-standard directories.

Copy link
Member

@robnagler robnagler left a comment

Choose a reason for hiding this comment

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

Thanks for checking for bugs in black.

pykern/pkcli/ci.py Outdated Show resolved Hide resolved
pykern/pkcli/fmt.py Outdated Show resolved Hide resolved
pykern/pkcli/fmt.py Outdated Show resolved Hide resolved
pykern/pkcli/fmt.py Outdated Show resolved Hide resolved
@rorour
Copy link
Contributor Author

rorour commented Dec 23, 2022

Do we want pykern fmt run . to only run on , tests, setup.py when run from the repo root?

Correct. We don't want to enforce standards on non-standard directories.

How should fmt run handle this? It seems like if fmt run is called from the command line we need to check each path that it receives and run it through ci._paths()?

@robnagler
Copy link
Member

It seems like if fmt run is called from the command line we need to check each path that it receives and run it through ci._paths()?

fmt is a utility that abstracts our formatter. It might eventually know how to run JS formatter, for example. It does not need to know about repos except that it does need to know about exclusions since it delegates tree walking to the formatter, which is typical of formatters.

ci abstracts continuous integration, which includes formatting and other standards (check prints). ci is what enforces standards on a repo so it sets standards by calling fmt only where we set standards (tests, setup.py, etc.). Therefore, fmt doesn't need to be changed except to accept a list of files instead of a single directory.

@rorour
Copy link
Contributor Author

rorour commented Dec 23, 2022

Then should we add a new command to ci to replace our widespread use of pykern fmt run .?

@robnagler
Copy link
Member

Then should we add a new command to ci to replace our widespread use of pykern fmt run .?

Not sure what you mean. Developers can still make sure the whole repo is formatted.

I'm not sure it is very widespread. For example, I never use pykern fmt run ., because I run pykern fmt run <file-saved>. I'm pretty sure Evan and Paul use the same command. I suspect VS code and PyCharm could do the same. This way there's no need to run fmt manually, which means there won't be ci failures for people forgetting to run pykern fmt run . manually.

Perhaps I'm misunderstanding.

@rorour
Copy link
Contributor Author

rorour commented Dec 24, 2022

Do we want pykern fmt run . to only run on , tests, setup.py when run from the repo root?

Correct. We don't want to enforce standards on non-standard directories.

This comment made me think you wanted some kind of change to the way fmt run currently works

@robnagler
Copy link
Member

This comment made me think you wanted some kind of change to the way fmt run currently works

Ah, no. fmt enforces standards, of course.

@rorour rorour requested a review from robnagler December 27, 2022 01:32
pykern/pkcli/ci.py Outdated Show resolved Hide resolved
pykern/pkcli/ci.py Show resolved Hide resolved
@rorour rorour requested a review from robnagler December 27, 2022 03:59
setup.py Outdated Show resolved Hide resolved
tests/pkcli/fmt_data/check1.out/pkexcept Outdated Show resolved Hide resolved
pykern/pkcli/ci.py Outdated Show resolved Hide resolved
@rorour rorour requested a review from robnagler December 27, 2022 22:12
Copy link
Member

@robnagler robnagler left a comment

Choose a reason for hiding this comment

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

Great job sticking with the updates! Thanks.

@robnagler robnagler merged commit b94654c into master Dec 27, 2022
@robnagler robnagler deleted the black-update-version branch December 27, 2022 22:26
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