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

update contribute/style #155

Merged
merged 7 commits into from
Jan 4, 2021
Merged

update contribute/style #155

merged 7 commits into from
Jan 4, 2021

Conversation

sgouezel
Copy link
Contributor

@sgouezel sgouezel commented Jan 3, 2021

Based on a discussion with @fpvandoorn at leanprover-community/mathlib#5414 (comment), I have the impression that the style guide does not completely reflect our current style. I propose some modifications in the current PR based on my feeling of what these practices are. Feedback/improvements/clarifications are most welcome!

@kbuzzard
Copy link
Member

kbuzzard commented Jan 3, 2021

These changes look great to me. In particular the comment that lower case greek letters should be used for "generic types" (but sensible maths capital letters can be used for sensible maths objects) is long overdue and I think fits very well with what is happening right now (and is also a good system!)

@PatrickMassot
Copy link
Member

LGTM.

Copy link
Collaborator

@bryangingechen bryangingechen left a comment

Choose a reason for hiding this comment

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

Looks good to me too!

@fpvandoorn
Copy link
Member

As mentioned in the Zulip thread, I would prefer to not have the convention "a single tactic invocation per line". The rest LGTM.

templates/contribute/style.md Outdated Show resolved Hide resolved
@urkud
Copy link
Member

urkud commented Jan 4, 2021

Should we change "We generally restrict the use of dots to inductive types." in naming.md as well?

@sgouezel
Copy link
Contributor Author

sgouezel commented Jan 4, 2021

I have also made a few changes to naming.md as recommended by @urkud

Copy link
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

Copy link
Member

@fpvandoorn fpvandoorn left a comment

Choose a reason for hiding this comment

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

Looks good to me too. I'll merge it.

@fpvandoorn fpvandoorn merged commit e0eac81 into newsite Jan 4, 2021
@fpvandoorn fpvandoorn deleted the style branch January 4, 2021 20:47
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.

None yet

7 participants