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

Complete the relocation of contrib HVect into base as All #3191

Merged
merged 7 commits into from
Mar 19, 2024

Conversation

mattpolzin
Copy link
Collaborator

@mattpolzin mattpolzin commented Jan 8, 2024

Description

Per discussion in https://github.com/idris-lang/Idris2/wiki/%5BLanguage%5D-Contrib-Organisation, the Data.HVect contrib module should be deprecated (or removed, I'd argue, since we can entirely replace it with base library's All and an alias).

I moved a number of functions from contrib over to base. A few of these functions changed names or argument orders but a number more were moved with no changes beyond generalizing them to work with All instead of just HVect. Where names were changed, the name changes were motivated by matching same-purpose functions in Data.Vect or Data.Vect.Elem.

This PR will cause Frex builds to fail until frex-project/idris-frex#69 is merged.

Should this change go in the CHANGELOG?

  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG_NEXT.md (and potentially also
    CONTRIBUTORS.md).

@mattpolzin mattpolzin changed the title complete the relocation of contrib HVect into base as All Complete the relocation of contrib HVect into base as All Jan 8, 2024
@mattpolzin mattpolzin marked this pull request as ready for review January 8, 2024 02:43
@andrevidela
Copy link
Collaborator

The change looks good to me but I'm curious about the upgrade path for frex. Do you mean to merge both the frex PR and this one at the same time? Or should frex be pinned to an older version for a bit? Or should we make the additive change to base only without removing contrib to make the tests pass?

@mattpolzin
Copy link
Collaborator Author

Do you mean to merge both the frex PR and this one at the same time?

that was my thought since we do that with e.g. pack and it’s the easiest option.

Or should frex be pinned to an older version for a bit?

I’m not sure now that you mention it if there’s any concept of pinning for frex; I think there might be an assumption that it always works with latest Idris. We could say/document that frex targeted 0.7.0 for now, which may as well mean we comment out the unit tests in the Idris compiler.

Or should we make the additive change to base only without removing contrib to make the tests pass?

that sounds unfortunate because then we’d have half-migrated the code, created possible ambiguity errors between base/contrib code that would only exist during that interim period, and we would be dependent on Frex merging a PR before we could complete the migration, whereas I’ve not heard any feedback on the frex PR so I dont yet know if/when that PR would be merged.

@andrevidela
Copy link
Collaborator

I see, probably best to get someone who has merge rights on frex to greenlight this then. I think both @gallais and @ohad qualify for this

@mattpolzin
Copy link
Collaborator Author

This process has begun. Frex merged a PR that works against this branch. Once this is merged, Frex's CI will be switched back to pointing at the main branch of Idris2.

@mattpolzin mattpolzin merged commit 7ce4c45 into idris-lang:main Mar 19, 2024
22 checks passed
@mattpolzin mattpolzin deleted the contrib-hvect-removal branch March 19, 2024 13:22
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.

2 participants