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] - fix(*): fix many indentation mistakes #10163

Closed
wants to merge 16 commits into from

Conversation

jcommelin
Copy link
Member

@jcommelin jcommelin commented Nov 4, 2021


Open in Gitpod

@jcommelin jcommelin added the awaiting-review The author would like community review of the PR label Nov 4, 2021
@github-actions github-actions bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Nov 4, 2021
src/data/buffer/parser/basic.lean Outdated Show resolved Hide resolved
src/data/rbtree/basic.lean Outdated Show resolved Hide resolved
apply linear_equiv.surjective,
rw [←linear_map.range_eq_top, finsupp.range_total],
simpa using s, },
-- We construct an surjective linear map `(w → R) →ₗ[R] (ι → R)`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-- We construct an surjective linear map `(w → R) →ₗ[R] (ι → R)`,
-- We construct a surjective linear map `(w → R) →ₗ[R] (ι → R)`,

Copy link
Member Author

Choose a reason for hiding this comment

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

This would destroy the "this PR only changes whitespace" property of this PR. Do we care?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to keep the changes whitespace-only, as there is enough to review in the linter itself :)

jcommelin and others added 2 commits November 4, 2021 15:38
Co-authored-by: sgouezel <sebastien.gouezel@univ-rennes1.fr>
@github-actions github-actions bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Nov 4, 2021
scripts/lint-style.py Outdated Show resolved Hide resolved
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: tb65536 <tb65536@users.noreply.github.com>
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Nov 8, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Nov 8, 2021
@Vierkantor Vierkantor self-requested a review November 9, 2021 10:49
Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Do I understand correctly that the indentation changes included in this PR are exactly those flagged by the linter, and there are no false positives you noticed? In that case, I'm happy to approve the overall design of the linter for now, and see how it responds to new code.

I suspect that doing the linter line-by-line is going to quickly make it hard to adapt if we need to keep tweaking it (already I don't really understand the order in which the different tokens in a line are handled): a better architecture would be to parse the file into tokens and advance the state token-by-token. That should make it easier to deal with situations like:

def blah := foo (some_complicated - expression) begin
  correctly_indented_tac,
  { subtac },
  { another_subtac }
end

scripts/lint-style.py Outdated Show resolved Hide resolved
scripts/lint-style.py Outdated Show resolved Hide resolved
scripts/lint-style.py Outdated Show resolved Hide resolved
Comment on lines 179 to 184
if line.find("begin") > 0 and in_prf == 0:
# complicate proof block syntax
check_rest_of_block = False
if lstr.find("begin") > 0:
# complicate proof block syntax
check_rest_of_block = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have an example of a "normal" piece of Lean where the second if would apply that is not handled by the first if?

Suggested change
if line.find("begin") > 0 and in_prf == 0:
# complicate proof block syntax
check_rest_of_block = False
if lstr.find("begin") > 0:
# complicate proof block syntax
check_rest_of_block = False
# Don't check complicated proof block syntax:
# - a `begin` block in the middle of term mode
if line.find("begin") > 0 and in_prf == 0:
check_rest_of_block = False
# - another case where `begin` appears somewhere else in the line
if lstr.find("begin") > 0:
check_rest_of_block = False

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 think the first if can just be removed. I don't want to check the indentation of subproofs of the form

have foo : bla := begin
  stuff
end,

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, nope. I clarified how they both do useful things in the other PR.

scripts/lint-style.py Outdated Show resolved Hide resolved
scripts/lint-style.py Outdated Show resolved Hide resolved
check_rest_of_block = True
ended_with_comma = False
inside_special = 0
if "match" in line or "calc" in line:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm concerned checks like if "keyword" in line are too likely to get false positives, e.g. the check for end will match all lines containing linear_independent, and cause the in_prf counter to turn negative. (It's good that you explicitly say in_prf > 0 above rather than in_prf, or we'd get some weird errors!)

More robust would be to use re.match("\bkeyword\b", line) (where \b is regex syntax for "word boundary").

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in the other PR

indent_lvl -= 2
in_prf -= 1
indent_lvl += 2 * line.count('{') # potential innocent(?) clash with set-builder notation
indent_lvl -= 2 * line.count('}') # there can be multiple closing braces on one line
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the motivation for checking [/] indent level before the "reset" logic and {/} indent level afterwards?

Copy link
Member Author

Choose a reason for hiding this comment

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

[/] is checking the nesting level, and if we're nested inside such a special block we don't want to do anything.
Moving these indent_lvl lines for {/} would break the indent checking code in the lines above.

scripts/lint-style.py Outdated Show resolved Hide resolved
apply linear_equiv.surjective,
rw [←linear_map.range_eq_top, finsupp.range_total],
simpa using s, },
-- We construct an surjective linear map `(w → R) →ₗ[R] (ι → R)`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to keep the changes whitespace-only, as there is enough to review in the linter itself :)

@Vierkantor Vierkantor added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Nov 9, 2021
jcommelin and others added 2 commits November 9, 2021 14:31
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@jcommelin jcommelin added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Nov 10, 2021
@PatrickMassot
Copy link
Member

bors merge

@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Nov 10, 2021
bors bot pushed a commit that referenced this pull request Nov 10, 2021
@sgouezel
Copy link
Collaborator

bors r+

bors bot pushed a commit that referenced this pull request Nov 10, 2021
@bors
Copy link

bors bot commented Nov 10, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(*): fix many indentation mistakes [Merged by Bors] - fix(*): fix many indentation mistakes Nov 10, 2021
@bors bors bot closed this Nov 10, 2021
@bors bors bot deleted the indentation-fixes branch November 10, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants