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] - feat: to_additive #234

Closed
wants to merge 79 commits into from
Closed

[Merged by Bors] - feat: to_additive #234

wants to merge 79 commits into from

Conversation

EdAyers
Copy link
Member

@EdAyers EdAyers commented Mar 17, 2022

By @fpvandoorn.

Depends on #129
Closes #229

@EdAyers EdAyers changed the title To additive attr feat: to_additive Mar 17, 2022
@gebner
Copy link
Member

gebner commented Apr 28, 2022

Is there still some meaning to the draft status of this PR (given that you've remove the WIP label)?

Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/ToAdditive.lean Outdated Show resolved Hide resolved
Co-authored-by: Gabriel Ebner <gebner@gebner.org>
@EdAyers
Copy link
Member Author

EdAyers commented May 4, 2022

The current status of this PR is that it is feature complete and I can get most examples to work, however there are still things to do:

  • add more examples and unit tests
  • fiddle with Group/Defs.lean to make it work well with to_additive
  • port documentation
  • add new name-guessing code for camelCase names
  • code style issues
  • there are still bugs

@EdAyers
Copy link
Member Author

EdAyers commented May 11, 2022

It might be a good idea to merge this sooner rather than when it is at feature parity and bug free. I think the bugs and features that we really need will become apparent once porting is underway.

@EdAyers EdAyers marked this pull request as ready for review May 11, 2022 05:00
Mathlib/Lean/Exception.lean Show resolved Hide resolved
Comment on lines +5 to +7
match ← tryCatch (m *> pure none) (pure ∘ some) with
| none => throwError "Expected an exception."
| some ex => return ex
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is clearer:

Suggested change
match ← tryCatch (m *> pure none) (pure ∘ some) with
| none => throwError "Expected an exception."
| some ex => return ex
try
discard m
throwError "Expected an exception."
catch ex =>
return ex

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm I think this doesn't quite work because it needs to throw if m doesn't throw

Mathlib/Lean/NameMapAttribute.lean Show resolved Hide resolved
@@ -64,11 +64,11 @@ is rewritten to: op (↑(↑(x : α) : β) : γ) (↑(y : β) : γ)
when (↑(↑(x : α) : β) : γ) = (↑(x : α) : γ) can be proven with a squash lemma
-/
def splittingProcedure (expr : Expr) : MetaM Simp.Result := do
let Expr.app (Expr.app op x ..) y .. := expr | return {expr}
let Expr.app (Expr.app op x ..) y .. := expr | return {expr := expr}
Copy link
Member

Choose a reason for hiding this comment

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

We really need to figure out something to disambiguate the set and record notations.

@gebner
Copy link
Member

gebner commented May 11, 2022

It might be a good idea to merge this sooner rather than when it is at feature parity and bug free. I think the bugs and features that we really need will become apparent once porting is underway.

I completely agree.

bors d+

@bors
Copy link

bors bot commented May 11, 2022

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

@EdAyers
Copy link
Member Author

EdAyers commented May 11, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 11, 2022
By @fpvandoorn.

Depends on #129 
Closes #229 

Co-authored-by: E.W.Ayers <edward.ayers@outlook.com>
@bors
Copy link

bors bot commented May 11, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: to_additive [Merged by Bors] - feat: to_additive May 11, 2022
@bors bors bot closed this May 11, 2022
@bors bors bot deleted the toAdditiveAttr branch May 11, 2022 23:43
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.

port: to_additive attribute
3 participants