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(Topology/Separation): rename type variables #7589

Closed
wants to merge 11 commits into from

Conversation

grunweg
Copy link
Collaborator

@grunweg grunweg commented Oct 9, 2023

This file was using a mix of Greek letters and standard math convention (X, Y, Z).
connectedComponent_eq_iInter_clopen was even using X in comments and $\alpha$ in the code.

As discussed on zulip, standardize on the latter.

@grunweg grunweg added awaiting-review The author would like community review of the PR t-topology Topological spaces, uniform spaces, metric spaces, filters labels Oct 9, 2023
@eric-wieser
Copy link
Member

eric-wieser commented Oct 9, 2023

Please make sure to put your description above the ---, otherwise it will be thrown away and not put in the commit message.

The space below the --- should be left for information that is only interesting before the PR is merged (such as "I need help" or "I'm waiting on this PR" or "Reviewers, what do you think of [alternate design]?")

@eric-wieser
Copy link
Member

eric-wieser commented Oct 10, 2023

Note my comments above are far from exhaustive: I just looked for a few examples that were particularly messy. It's certainly not a requirement that you catch all of them, but you're making a tradeoff here between "Greek letters are weird" and "some variables have actively misleading names", and you need to ensure it's not so many that the change is a net downgrade.

@grunweg
Copy link
Collaborator Author

grunweg commented Oct 10, 2023

I hear your concern and agree this is important. I've checked all of the following to catch as many instances as possible. (This covers all instances you pointed out, and also the comments in #7587.)

  • before renaming: audit misleading type names: are X, Y, Z used as variables elsewhere? If so, rename those or convince myself it's fine.

  • did I catch all type variables?

    • search for Type and Type*; audit all results
    • types $\iota, \pi$ for indexing is fine
    • use X_i, Y_i etc. for a collection of topological spaces
  • rename all occurrences; this ensure documentation is also updated

  • generalising universes: try generalising a Type u to Type* (revert if the build fails)
    (Do this in a separate commit, so this can be dropped if needed.)

  • Re-use variables if universes match: in theorems only; leave definitions, classes, structures and instances alone.
    (If names were mixed before, renaming may allow to unify things.)

  • misleading variable names: all of x : Y, x : Z, y : X, y : Z.
    Often, I can audit all occurrences of z, Z or even y and Y (even in proofs).

  • now-suboptimal variable names: a : X etc. (Perhaps, I can even audit all uses of the variable a.)

@grunweg
Copy link
Collaborator Author

grunweg commented Oct 10, 2023

I am not sure if the last commit is actually a net positive: certainly, the variable name a is used pervasively. I'm open to dropping this one.

Copy link
Contributor

@sgouezel sgouezel left a comment

Choose a reason for hiding this comment

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

bors d+
Thanks!

Mathlib/Topology/Separation.lean Outdated Show resolved Hide resolved
Mathlib/Topology/Separation.lean Outdated Show resolved Hide resolved
@bors
Copy link

bors bot commented Oct 30, 2023

✌️ grunweg can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added delegated and removed awaiting-review The author would like community review of the PR labels Oct 30, 2023
@grunweg
Copy link
Collaborator Author

grunweg commented Oct 30, 2023

Thank you for the review. I have addressed both nits (as well as another analogous case in this file.)
bors r+

bors bot pushed a commit that referenced this pull request Oct 30, 2023
This file was using a mix of Greek letters and standard math convention (X, Y, Z).
`connectedComponent_eq_iInter_clopen` was even using `X` in comments and $\alpha$ in the code.

As [discussed](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Naming.20convention.3A.20topological.20spaces/near/395769893) on zulip, standardize on the latter.
@bors
Copy link

bors bot commented Oct 30, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title chore(Topology/Separation): rename type variables [Merged by Bors] - chore(Topology/Separation): rename type variables Oct 30, 2023
@bors bors bot closed this Oct 30, 2023
@bors bors bot deleted the MR-rename-types3a branch October 30, 2023 19:25
grunweg added a commit that referenced this pull request Nov 1, 2023
This file was using a mix of Greek letters and standard math convention (X, Y, Z).
`connectedComponent_eq_iInter_clopen` was even using `X` in comments and $\alpha$ in the code.

As [discussed](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Naming.20convention.3A.20topological.20spaces/near/395769893) on zulip, standardize on the latter.
fgdorais pushed a commit that referenced this pull request Nov 1, 2023
This file was using a mix of Greek letters and standard math convention (X, Y, Z).
`connectedComponent_eq_iInter_clopen` was even using `X` in comments and $\alpha$ in the code.

As [discussed](https://leanprover.zulipchat.com/#narrow/stream/287929-mathlib4/topic/Naming.20convention.3A.20topological.20spaces/near/395769893) on zulip, standardize on the latter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated t-topology Topological spaces, uniform spaces, metric spaces, filters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants