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: fix more casing errors per naming scheme #1232

Closed
wants to merge 23 commits into from

Conversation

winstonyin
Copy link
Collaborator

@winstonyin winstonyin commented Dec 27, 2022

I've avoided anything under Tactic or test.

In correcting the names, I found Option.isNone_iff_eq_none duplicated between Std and Mathlib, so the Mathlib one has been removed.

@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 28, 2022
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Dec 31, 2022
Copy link
Member

@hrmacbeth hrmacbeth left a comment

Choose a reason for hiding this comment

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

I haven't looked at it all, just commenting on a few that stood out to me. Feel free to open Zulip discussions in ambiguous cases.

@@ -240,7 +240,7 @@ instance (priority := 70) (R : Type _) [e : EuclideanDomain R] : NoZeroDivisors

-- see Note [lower instance priority]
instance (priority := 70) (R : Type _) [e : EuclideanDomain R] : IsDomain R :=
{ e, NoZeroDivisors.toIsDomain R with }
{ e, NoZeroDivisors.to_isDomain R with }
Copy link
Member

Choose a reason for hiding this comment

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

The to might be a special case? See the example MonoidHom.toOneHom_injective in the naming convention guide.

Copy link
Collaborator Author

@winstonyin winstonyin Jan 2, 2023

Choose a reason for hiding this comment

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

My understanding is, since IsDomain is a Prop, to_isDomain should follow the convention for proofs, i.e. snake case. On the other hand, toOneHom contains data, so its name should be lower camel case.

Mathlib/Algebra/Group/Conj.lean Outdated Show resolved Hide resolved
Mathlib/Algebra/Order/Monoid/Defs.lean Outdated Show resolved Hide resolved
@hrmacbeth hrmacbeth added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review labels Jan 2, 2023
@winstonyin winstonyin added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jan 2, 2023
@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 3, 2023
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 3, 2023
@rwbarton
Copy link
Contributor

rwbarton commented Jan 3, 2023

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Jan 3, 2023
bors bot pushed a commit that referenced this pull request Jan 3, 2023
I've avoided anything under `Tactic` or `test`.

In correcting the names, I found `Option.isNone_iff_eq_none` duplicated between `Std` and `Mathlib`, so the `Mathlib` one has been removed.

Co-authored-by: Reid Barton <rwbarton@gmail.com>
@bors
Copy link

bors bot commented Jan 3, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore: fix more casing errors per naming scheme [Merged by Bors] - chore: fix more casing errors per naming scheme Jan 3, 2023
@bors bors bot closed this Jan 3, 2023
@bors bors bot deleted the winstonyin_typos branch January 3, 2023 22:36
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

4 participants