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(Order.Grade): remove porting notes #10496

Closed
wants to merge 2 commits into from

Conversation

mattrobball
Copy link
Collaborator

Simple changes to remove porting notes.


Open in Gitpod

@mattrobball mattrobball added awaiting-CI awaiting-review The author would like community review of the PR easy < 20s of review time. See the lifecycle page for guidelines. labels Feb 13, 2024
@j-loreaux
Copy link
Collaborator

According to the note:

-- porting note - what the hell?! used to be grade 𝕆

it seems that grade should have an explicit argument, but in your code you just have grade, so it's implicit. Or am I missing something?

@j-loreaux j-loreaux 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 Feb 13, 2024
@mattrobball
Copy link
Collaborator Author

Yes. I guess we could try to change the binder type in grade to be explicit but this seemed ok otherwise.

@Ruben-VandeVelde
Copy link
Collaborator

Ruben-VandeVelde commented Feb 13, 2024

According to the note:

-- porting note - what the hell?! used to be grade 𝕆

it seems that grade should have an explicit argument, but in your code you just have grade, so it's implicit. Or am I missing something?

What happened here is that GradeOrder.grade takes an implicit 𝕆 and _root_.grade takes an explicit 𝕆. Definitions in the GradeOrder namespace now pick GradeOrder.grade. We could use

class GradeOrder (𝕆 α : Type*) [Preorder 𝕆] [Preorder α] where
  /-- The grading function. -/
  protected grade : α → 𝕆

to avoid this

@mattrobball
Copy link
Collaborator Author

I used _root_.grade in those two locations.

Making it protected is sensible but out of scope for this PR. I would be happy if there is a separate one doing this.

@mattrobball mattrobball 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 easy < 20s of review time. See the lifecycle page for guidelines. labels Feb 13, 2024
@j-loreaux
Copy link
Collaborator

bors merge

@github-actions github-actions bot 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 Feb 13, 2024
mathlib-bors bot pushed a commit that referenced this pull request Feb 13, 2024
Simple changes to remove porting notes.
@mattrobball
Copy link
Collaborator Author

Note: the first porting note was an instance of leanprover/lean4#2789

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Feb 13, 2024

Build failed (retrying...):

mathlib-bors bot pushed a commit that referenced this pull request Feb 13, 2024
Simple changes to remove porting notes.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Feb 13, 2024

Build failed (retrying...):

@mattrobball
Copy link
Collaborator Author

bors r-

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Feb 13, 2024

Canceled.

@mattrobball
Copy link
Collaborator Author

bors merge

mathlib-bors bot pushed a commit that referenced this pull request Feb 14, 2024
Simple changes to remove porting notes.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Feb 14, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore(Order.Grade): remove porting notes [Merged by Bors] - chore(Order.Grade): remove porting notes Feb 14, 2024
@mathlib-bors mathlib-bors bot closed this Feb 14, 2024
@mathlib-bors mathlib-bors bot deleted the mrb/order_grade_porting_notes branch February 14, 2024 05:27
riccardobrasca pushed a commit that referenced this pull request Feb 18, 2024
Simple changes to remove porting notes.
mathlib-bors bot pushed a commit that referenced this pull request Feb 23, 2024
In #10496, a better fix was suggested in the comments by @Ruben-VandeVelde: make `GradedOrder.grade` `protected` to freely use `grade` for `_root_.grade`. This implements that fix.
thorimur pushed a commit that referenced this pull request Feb 24, 2024
In #10496, a better fix was suggested in the comments by @Ruben-VandeVelde: make `GradedOrder.grade` `protected` to freely use `grade` for `_root_.grade`. This implements that fix.
thorimur pushed a commit that referenced this pull request Feb 26, 2024
In #10496, a better fix was suggested in the comments by @Ruben-VandeVelde: make `GradedOrder.grade` `protected` to freely use `grade` for `_root_.grade`. This implements that fix.
riccardobrasca pushed a commit that referenced this pull request Mar 1, 2024
In #10496, a better fix was suggested in the comments by @Ruben-VandeVelde: make `GradedOrder.grade` `protected` to freely use `grade` for `_root_.grade`. This implements that fix.
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
Simple changes to remove porting notes.
dagurtomas pushed a commit that referenced this pull request Mar 22, 2024
In #10496, a better fix was suggested in the comments by @Ruben-VandeVelde: make `GradedOrder.grade` `protected` to freely use `grade` for `_root_.grade`. This implements that fix.
Louddy pushed a commit that referenced this pull request Apr 15, 2024
In #10496, a better fix was suggested in the comments by @Ruben-VandeVelde: make `GradedOrder.grade` `protected` to freely use `grade` for `_root_.grade`. This implements that fix.
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

3 participants