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] - refactor(*): add a notation for nhds_within #3683

Closed
wants to merge 4 commits into from

Conversation

urkud
Copy link
Member

@urkud urkud commented Aug 4, 2020

The definition is still there and can be used too.


@urkud urkud added the RFC Request for comment label Aug 4, 2020
@sgouezel
Copy link
Collaborator

sgouezel commented Aug 4, 2020

I'm fine with the notation, but I think I'd rather keep the definition: one shouldn't need to know what principal means to be able to use neighborhood_within, and the definition hides it away.

@PatrickMassot
Copy link
Member

I'm fine with the notation, but I think I'd rather keep the definition: one shouldn't need to know what principal means to be able to use neighborhood_within, and the definition hides it away.

What I've seen in this PR doesn't suggest that it requires to know more about the definition. And I really like the new notation (but I think everybody agrees here). I'd merge this.

@urkud
Copy link
Member Author

urkud commented Aug 4, 2020

I'm fine with the notation, but I think I'd rather keep the definition: one shouldn't need to know what principal means to be able to use neighborhood_within, and the definition hides it away.

What I've seen in this PR doesn't suggest that it requires to know more about the definition. And I really like the new notation (but I think everybody agrees here). I'd merge this.

You need to know that this is a notation, not a definition in order to understand why simp [← nhds_within_univ] fails. I can revert parts of the PR that remove the definition.

@sgouezel
Copy link
Collaborator

sgouezel commented Aug 4, 2020

Since Patrick likes it the way it is, I can live with it. So, Yury, do as you think is best, and then you can merge it.
bors d+

@bors
Copy link

bors bot commented Aug 4, 2020

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

@PatrickMassot
Copy link
Member

I really don't have a strong opinion here, but I haven't seen anything shocking in the new proofs, and I also trust Yury.

@urkud
Copy link
Member Author

urkud commented Aug 5, 2020

and I also trust Yury.

I think that notation + def vs notation matters mostly for new users, and you shouldn't trust me here. I definitely can live with any of these options. Here is the differences I spotted while porting the code:

  • with notation + def if users want to rw/simp using a lemma expecting f ⊓ 𝓟 s, then they need to simp/dsimp/dunfold/rw nhds_within first.
  • with notation (no def) users can't do simp [← nhds_within_univ] anymore and sometimes they can be surprised by simp/rw rewriting nhds part of nhds_within notation.

TL;DR: I agree with @sgouezel that notation + def has less surprising behavior and unfolding nhds_within when we want it is not hard, so I should revert part of my PR.

@urkud
Copy link
Member Author

urkud commented Aug 5, 2020

I had a green light with the previous version and built the new version (some changes reverted) locally, so
bors merge

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Aug 5, 2020
@urkud urkud removed the RFC Request for comment label Aug 5, 2020
@urkud
Copy link
Member Author

urkud commented Aug 5, 2020

bors r-

@bors
Copy link

bors bot commented Aug 5, 2020

Canceled.

@urkud urkud changed the title refactor(*): replace def nhds_within by a notation refactor(*): add a notation for nhds_within Aug 5, 2020
@urkud
Copy link
Member Author

urkud commented Aug 5, 2020

bors merge

bors bot pushed a commit that referenced this pull request Aug 5, 2020
The definition is still there and can be used too.
@bors
Copy link

bors bot commented Aug 5, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(*): add a notation for nhds_within [Merged by Bors] - refactor(*): add a notation for nhds_within Aug 5, 2020
@bors bors bot closed this Aug 5, 2020
@bors bors bot deleted the nhds-within branch August 5, 2020 09:55
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

3 participants