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

Refactor library to use λ where #809

Merged
merged 27 commits into from
Oct 9, 2023
Merged

Conversation

fredrik-bakke
Copy link
Collaborator

@fredrik-bakke fredrik-bakke commented Sep 29, 2023

Also adds a new rule removing blank lines right before where keywords.

@fredrik-bakke
Copy link
Collaborator Author

I also remembered that we don't actually have a script focused on best practices for agda code, and I won't bother making one now just for this convention.

@fredrik-bakke fredrik-bakke changed the title λ where Refactor library to use λ where Oct 8, 2023
@fredrik-bakke
Copy link
Collaborator Author

This one's ready now. At some point in the future, when there are fewer pull requests in the pipeline, I'd like to go through the repository to change most instances of pair with _,_ using replace patterns like

  • (?<!-)pair (?!of)([^`()\s]+)( |$) -> $1 ,$2

@fredrik-bakke fredrik-bakke marked this pull request as ready for review October 8, 2023 20:13
@EgbertRijke
Copy link
Collaborator

This one's ready now. At some point in the future, when there are fewer pull requests in the pipeline, I'd like to go through the repository to change most instances of pair with _,_ using replace patterns like

  • (?<!-)pair (?!of)([^`()\s]+)( |$) -> $1 ,$2

Aren't there some instances where pair is nicer than _,_?

@fredrik-bakke
Copy link
Collaborator Author

fredrik-bakke commented Oct 8, 2023

I agree, when both arguments span multiple lines*, I think pair is nicer than _,_. But I thought I saw you recently say that we always preferred the latter? Either way, the pattern I presented only matches instances where pair and its first argument are on the same line. In this case I think _,_ will generally be nicer. I don't feel too strongly about cases where we have

 x , 
 y
   z
   w

So if you want, I can restrict to only replacing instances where both arguments of pair appear on the same line.

@EgbertRijke
Copy link
Collaborator

EgbertRijke commented Oct 8, 2023

I feel like we should still include some guidelines about what options we have to type-set lambdas. Eg we could mention that we always wrap the lambda-expression in parentheses, even if it is the last argument of a function and therefore not strictly required. Then we could mention that there are several lambda-styles available:

  1. regular lambda
  2. pattern-matching lambda without { } as in lambda (x , y) -> ...
  3. pattern-matching lambda with { }
  4. lambda-where

and we could mention that whenever lambdas 2-4 appear in an entry we mark it as abstract. We could also mention that while these fancy lambdas are permitted in our code, in many common situations it might be preferable to factor out that lambda-expression into a separate definition. And then we could also say that the formatting guidelines for these lambdas are not quite cleared up yet, just to be upfront.

@EgbertRijke
Copy link
Collaborator

@VojtechStep also mentioned some lambda-styles that we don't use in our library. I already forgot about them, but we could include those too in our style guide and say that we don't use them.

@VojtechStep
Copy link
Collaborator

2. pattern-matching lambda without { } as in lambda (x , y) -> ...

I don't think this is technically a pattern-matching lambda - it's only allowed for record types with eta equality (which, to be fair, covers many of our use cases, except path induction).

src/foundation/0-connected-types.lagda.md Outdated Show resolved Hide resolved
src/group-theory/generating-elements-groups.lagda.md Outdated Show resolved Hide resolved
src/set-theory/cumulative-hierarchy.lagda.md Outdated Show resolved Hide resolved
scripts/blank_line_conventions.py Show resolved Hide resolved
src/foundation/identity-types.lagda.md Outdated Show resolved Hide resolved
@EgbertRijke
Copy link
Collaborator

The conventions you use for lambda-where look very reasonable. They may not exactly fit uniformly in our indentation conventions, but I think we can stick with them if we explain it in the guidelines.

@fredrik-bakke
Copy link
Collaborator Author

Am I correct that @VojtechStep intended to update the guidelines regarding pattern-matching lambdas, or did you want me to do it here?

@EgbertRijke
Copy link
Collaborator

I don't know if Vojtech was planning to do that, but since this PR introduces lambda-where globally to the library, the natural PR to include some formatting guidelines seems to be this one.

@fredrik-bakke
Copy link
Collaborator Author

Okay, that makes sense. And then I guess he will write the guidelines for the other proposed instance where he wanted to use pattern-matching lambdas.

@fredrik-bakke fredrik-bakke changed the title Refactor library to use lambda where Refactor library to use λ where Oct 8, 2023
@fredrik-bakke
Copy link
Collaborator Author

I changed the title back because I don't think it should cause trouble. If it does, then we need to address that another way in any case I think.

@EgbertRijke
Copy link
Collaborator

Man, type-checking the whole library is getting quite slow

Strange! Do you know why it is getting slow?

@fredrik-bakke
Copy link
Collaborator Author

IDK, my guess is a large code base. Maybe it would be a good idea to keep a log of type-checking time for the whole library after each PR to keep a watch on it

@EgbertRijke
Copy link
Collaborator

IDK, my guess is a large code base. Maybe it would be a good idea to keep a log of type-checking time for the whole library after each PR to keep a watch on it

Do you mean that it is generally taking a long time to compile the library, or that something in this PR happened that makes it take longer to compile?

@fredrik-bakke
Copy link
Collaborator Author

Hard for me to say if it's gotten slower with this PR. It may just be that I'm type-checking a lot while working on this PR, hence noticing the slow

@fredrik-bakke
Copy link
Collaborator Author

Let me know what you think of the new convention

CODINGSTYLE.md Outdated Show resolved Hide resolved
@EgbertRijke
Copy link
Collaborator

Let me know what you think of the new convention

Beautiful!

@EgbertRijke EgbertRijke merged commit a29e25d into UniMath:master Oct 9, 2023
4 checks passed
@fredrik-bakke fredrik-bakke deleted the λ-where branch October 9, 2023 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants