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

Moving Data.List.HasLength into base #2844

Merged
merged 4 commits into from
Jan 16, 2023

Conversation

mattpolzin
Copy link
Collaborator

@mattpolzin mattpolzin commented Jan 8, 2023

Per the conclusion documented in https://github.com/idris-lang/Idris2/wiki/%5BLanguage%5D-Contrib-Organisation, Data.List.HasLength is being moved into the base library and removed from the contrib library.

With the move, I took the liberty of adopting some naming from contrib that I liked (will result in small refactoring of the compiler library after the next release of the base library). I also took the liberty of adopting the type signature from the compiler as opposed to the one in contrib (they take parameters in reverse-order from each other). I figured the compiler codebase represented the more real-world use-case and gave the ergonomics that have worked well for the compiler codebase the win on this one.

As a side note, I would have loved to have used the %deprecate feature in the compiler codebase for the functions that will be removed next version, but we have warnings-as-errors turned on for the compiler build process (which is good) so we cannot produce deprecation warnings in that codebase!

I've sanity checked that removing all of the functions marked for removal after the next release will work by deleting them and building the Idris compiler locally against the new base library HasLength.

Should this change go in the CHANGELOG?

  • If this is a user-facing change or a compiler change, I have updated
    CHANGELOG.md (and potentially also CONTRIBUTORS.md).

@mattpolzin mattpolzin marked this pull request as ready for review January 8, 2023 04:03
@mattpolzin
Copy link
Collaborator Author

@gallais i thought you might have an opinion about the (re-)ordering of the parameters on the HasLength type.

If you don't have an opinion or objection, I will merge this soon.

||| f1 : (n : Nat) -> (0 _ : HasLength xs n) -> P xs
||| f2 : (n : Subset n (HasLength xs)) -> P xs
||| f1 : (n : Nat) -> (0 _ : HasLength n xs) -> P xs
||| f2 : (n : Subset n (flip HasLength xs)) -> P xs
Copy link
Member

Choose a reason for hiding this comment

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

I'd say it can be written (`HasLength` xs) but the punning does not work anymore with the new argument order. 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It risks being obvious, but "xs does have a length" ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I think I see what you mean. xs HasLength n used to read well.

Maybe HasLength n xs should have been named IsLength to remain reminiscent of English. I wouldn't vote for renaming now, though.

@mattpolzin mattpolzin merged commit 24ac56d into idris-lang:main Jan 16, 2023
@mattpolzin mattpolzin deleted the move-haslength-to-base branch January 16, 2023 06:07
@clayrat
Copy link
Contributor

clayrat commented Jan 27, 2023

Shouldn't the TODOs say v0.7.0?

@mattpolzin
Copy link
Collaborator Author

@clayrat The todos say “after 0.6.0” which was my intention in this case, whether that’s 0.6.1 or 0.7.0 or 1.0.0

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