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

Strip trailing spaces from informal descriptions #237

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented Oct 21, 2024

The changes to putnam.json are the result of a regex replace for \s*\\n with \\n.
These trailing spaces are stripped in the Lean docstrings by VSCode, which previously caused CI to invisibly fail when editing one of these "booby-trapped" files.

If changing the JSON file is not desirable, I can teach the CI script to remove the trailing spaces before comparison instead.

This is a regex replace for `\s*\\n` with `\\n`.
These trailing spaces are stripped in the Lean docstrings by VSCode, which causes CI to invisibly fail.

If changing the JSON file is not desirable, I can teach the CI script to remove the trailing spaces before comparison instead.
@amit9oct
Copy link
Collaborator

amit9oct commented Oct 21, 2024

This can be avoided just by checking the commit. The space was visible in PR clearly. It is possible that the logic for ignoring trailing spaces is needed in some corner case where we want to have trailing spaces in the informal description. The PR author can simply check the PR for spaces. I'm not sure if it is a great idea to complicate the validation check/CI logic for PRs.

@amit9oct
Copy link
Collaborator

These spaces would have been visible even before commit using git diff. One can also change VS code settings.

@eric-wieser
Copy link
Contributor Author

eric-wieser commented Oct 21, 2024

[...] in some corner case where we want to have trailing spaces in the informal description

I claim that no such situation exists; no reasonable dialect of LaTeX is sensitive to trailing spaces, and all of this text is LaTeX source.

These spaces would have been visible even before commit using git diff.

in the case of #234, there are >600 files, so this it not terribly viable.

One can also change VS code settings.

Either the trailing spaces should be stripped from the lean files in this repo, or the repo should come with config saying "don't strip trailing spaces". I claim that the first option is better, since there is no good reason to keep trailing spaces in the lean files.

@amit9oct
Copy link
Collaborator

amit9oct commented Oct 21, 2024

[...] in some corner case where we want to have trailing spaces in the informal description

I claim that no such situation exists; no reasonable dialect of LaTeX is sensitive to trailing spaces, and all of this text is LaTeX source.

I don't know if your claim covers all corner cases for example someone using pre formatted mode in LaTeX and some other cases which of course you and I are not able to anticipate now.

These spaces would have been visible even before commit using git diff.

in the case of #234, there are >600 files, so this it not terribly viable.

As per your comment on PR 216(#216) you mentioned that one can use git checkout -p. I pointed similar issues, but since only a few people are contributing so it can be handled by changing the VS Code config too.

In my opinion, the right way to do this have only one source of truth and use your script before merge to populate the comments directly in the Lean files. Otherwise accidental changes, difference in editor configs will lead to issues, or something which you cannot anticipate now will break the CI.

A much better solution rather than this complicated validation strategy is change the informal json to precisely match the comment. The easy fix will be to match the informal json to strip terminal spaces, this will keep check simple and easy for anyone editing these files. We will be robust with VS Code settings. The validation in CI will be simple and intuitive rather than some complicated regex match.

@eric-wieser
Copy link
Contributor Author

The easy fix will be to match the informal json to strip terminal spaces

Is this not precisely what the PR we are writing comments on already does?

@amit9oct
Copy link
Collaborator

If changing the JSON file is not desirable, I can teach the CI script to remove the trailing spaces before comparison instead.

My comments are about this.

Copy link
Collaborator

@GeorgeTsoukalas GeorgeTsoukalas left a comment

Choose a reason for hiding this comment

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

There seems to be agreement that this PR is the right way to go. Thanks for the contribution, Eric!

@GeorgeTsoukalas GeorgeTsoukalas merged commit 0be019d into trishullab:main Oct 22, 2024
1 check passed
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.

3 participants