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

[Merged by Bors] - chore: script for by linebreaks #1525

Closed
wants to merge 2 commits into from

Conversation

mo271
Copy link
Collaborator

@mo271 mo271 commented Jan 13, 2023

Not quite sure if the -i inplace part will work on all systems...

@mo271 mo271 added the awaiting-review The author would like community review of the PR label Jan 13, 2023
@mo271
Copy link
Collaborator Author

mo271 commented Jan 13, 2023

It might be good, as suggested by @digama0 to run this when generating the files for mathlib3port

@eric-wieser
Copy link
Member

Can mathport do this for us?

@mo271
Copy link
Collaborator Author

mo271 commented Jan 13, 2023

I think that would be very good!
In fact it might even do this for lines of length 98, 99 and 100, resulting in some linter warning, that the person porting can take care of manually in those cases, because with this script those cases are not taken care of.

@rwbarton
Copy link
Contributor

mathport used to get the by location correct (at least more of the time), it seems things changed in the run that resulted in leanprover-community/mathlib3port@27def98.
But the old version has a lot of weird layout in declarations, as well.
It's also not clear to me what actually changed to cause the new layout...

@rwbarton
Copy link
Contributor

Could you add a 1-2 line comment explaining what the script does (e.g. # Modify all lean files in mathlib to put "by" at the end of the previous line, where possible)?

@digama0
Copy link
Member

digama0 commented Jan 13, 2023

mathport used to get the by location correct (at least more of the time), it seems things changed in the run that resulted in leanprover-community/mathlib3port@27def98. But the old version has a lot of weird layout in declarations, as well. It's also not clear to me what actually changed to cause the new layout...

That run was mathport #544, and if you look at the CI page from around then you will notice that that run was preceded by mathport being broken for several days, during which there was a lean bump from lean4:nightly-2022-12-16 to lean4:nightly-2022-12-22. During that period the most likely commits that could have caused this are

@mo271
Copy link
Collaborator Author

mo271 commented Jan 14, 2023

Could you add a 1-2 line comment explaining what the script does (e.g. # Modify all lean files in mathlib to put "by" at the end of the previous line, where possible)?

Done

Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

bors merge

@semorrison semorrison added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Jan 19, 2023
bors bot pushed a commit that referenced this pull request Jan 19, 2023
Not quite sure if the `-i inplace` part will work on all systems...


Co-authored-by: Moritz Firsching <firsching@google.com>
@bors
Copy link

bors bot commented Jan 19, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore: script for by linebreaks [Merged by Bors] - chore: script for by linebreaks Jan 19, 2023
@bors bors bot closed this Jan 19, 2023
@bors bors bot deleted the mo271/linebreak_script branch January 19, 2023 07:45
jcommelin pushed a commit that referenced this pull request Jan 23, 2023
Not quite sure if the `-i inplace` part will work on all systems...


Co-authored-by: Moritz Firsching <firsching@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants