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

committed changes as per Issue #1589 #1592

Merged
merged 8 commits into from
Sep 29, 2021
Merged

committed changes as per Issue #1589 #1592

merged 8 commits into from
Sep 29, 2021

Conversation

jamesmckinna
Copy link
Contributor

New names in Data.Fin.Base , as discussed.
Knock-on consequences for Data.Fin.* Data.Vec.*, as discussed.
Passes fix-whitespace and GenerateEverything tests.
Old definitions not deleted, only deprecated.
Deprecate warning notices added.
CHANGELOG.md updated.

Copy link
Contributor

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

Otherwise, this is looking really good.

Copy link
Contributor

@MatthewDaggitt MatthewDaggitt left a comment

Choose a reason for hiding this comment

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

Thanks, looks like a great first PR! I've got a few minor niggly style comments, but apart from those looks great!

@jamesmckinna
Copy link
Contributor Author

I have pushed the changes to my local repo; how do I now add them to the PR. or is this automagically done?

@MatthewDaggitt
Copy link
Contributor

Automatically done, anything you push to the branch in your local repo gets added here 👍

@jamesmckinna
Copy link
Contributor Author

OK, cool. But did I catch every snag? I guess I'll find out... :-)

Copy link
Contributor

@MatthewDaggitt MatthewDaggitt left a comment

Choose a reason for hiding this comment

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

A rogue code snippets left, but other than that looks great to me! I usually leave PRs up for approximately 7 days to give more people the chance to comment even if they're busy. After that, I'll merge it in 👍

…for documentation/argument order shenanigans
@gallais
Copy link
Member

gallais commented Sep 24, 2021

We should probably pick fixities for these new definitions. The left/right associativity
is clearly set by the type but the level is more of an open design decision. It should
perhaps be tighter than _+_ and _*_ so that it interacts well with these Fin operations?

@JacquesCarette
Copy link
Contributor

See README/Design/Fixity.agda for notes on how fixities in the library are currently set.

@MatthewDaggitt
Copy link
Contributor

No more comments so merging in, thanks @jamesmckinna 👍

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.

Better names for Fin index manipulation functions in Data.Fin.Base
4 participants