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.

Sign up for GitHub

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

Jump to bottom

Fix #244. Instances for Data.Vector.Strict #247

Merged
merged 9 commits into from
Nov 28, 2024
Merged

Fix #244. Instances for Data.Vector.Strict #247

merged 9 commits into from
Nov 28, 2024

Conversation

jcmartin
Copy link
Contributor

@jcmartin jcmartin commented Nov 18, 2024 •
edited
Loading

This addresses the lack of instances for Data.Vector.Strict that were added in vector-0.13.2.0.

Tests have been added for the new instances and have passed for the following stackage releases:

  • nightly-2024-11-18
  • lts-22.42 (to confirm backwards compatibility)

@BebeSparkelSparkel

@Bodigrim This should help resolve mgsloan/store#179

@jcmartin jcmartin mentioned this pull request Nov 19, 2024
Merged
@BebeSparkelSparkel
Copy link
Collaborator

@jcmartin Thanks for the PR!

Requests:

  1. Related to Semigroup on GrowingAppend #246, could you add a date to the documentation of the instance when the cpp should be removed. 8 years seems a bit much though perhaps 4? Let me know what you think? Or, perhaps a version of ghc release?
  2. Can you add a warning in the tests indicating that the strict version is not being tested?

@BebeSparkelSparkel
Copy link
Collaborator

Also, I have created the mono-traversable-1.1.0.0 branch for breaking changes. Perhaps, this would be better included there so the CPP is not required.

Or perhaps, you want this in 1.0 and the cpp could be removed in 1.1

Let me know what you think.

@jcmartin
Copy link
Contributor Author

There are many ways to make a decision as to when to create a breaking change. I think the most important one is if you have an important feature or enhancement that is not reasonably possible without a newer version of a dependency. I don't consider adding instances to be a significant enough enhancement to create work for people.

Given that mono-traversable is directly and indirectly a dependency for lots of packages, it would make sense to take a conservative approach to this. GHC 9.4 is currently the "recommended" version and going on what I have heard from in the community GHC 9.4 and 9.6 are where most people are at this point.

Using the stackage releases as a guide, the first distribution to include the new vector library is ghc-9.8.3 in nightly. We could lose backwards compatibility once ghc-9.8.3 or higher has a significant "market share". Maybe a good rule of thumb should be providing support for the last three major releases available on stackage.

With all of this said, I would say we could require vector-0.13.2.0 or higher once ghc-9.12 is in nightly, or there is a significant change in mono-traversable warranting the need for newer dependencies.

@BebeSparkelSparkel What do you think?

@jcmartin
Copy link
Contributor Author

Also, the strict version is being tested if a new enough version of vector is included. Are you suggesting to include a message when the strict version is not being tested?

@BebeSparkelSparkel
Copy link
Collaborator

@gregwebs What do you think about removing the CPP as suggested above?

@jcmartin Yes, have a warning message in a CPP else.

@jcmartin
Copy link
Contributor Author

@BebeSparkelSparkel This is done.

@jcmartin
Copy link
Contributor Author

@BebeSparkelSparkel Can we have the discussion in parallel with merging this PR? This is currently holding up a pull request to fix the store package.

@BebeSparkelSparkel
Copy link
Collaborator

Just reviewed this last night. I'm going to make some mods today.

@BebeSparkelSparkel
feat: Add vector <0.13.2 deprecation notice and enable groupBy for st…
b2dba2d 
…rict vector

Adds deprecation warnings for vector <0.13.2 support and enables groupBy method for Vector. Changes:
- Add DependencyDeprecation pragma for vector <0.13.2
- Enable V.groupBy implementation that was commented out
- Replace runtime warning with pragma for missing VSC tests
@BebeSparkelSparkel
Copy link
Collaborator

@jcmartin please review the changes I have made

@jcmartin
Copy link
Contributor Author

@BebeSparkelSparkel Everything looks good. We could add the groupBy instances for the other vector types when we upgrade. I don't know if you want to leave a comment regarding that or wrap it up in macros to enable it now.

@BebeSparkelSparkel
Copy link
Collaborator

Leaving this for a different pr #251

@jcmartin
Copy link
Contributor Author

Actually, where are the pragmas DependencyDeprecation and MissingTests defined? I have not seen these before.

@BebeSparkelSparkel
Copy link
Collaborator

BebeSparkelSparkel commented Nov 27, 2024 •
edited
Loading

@jcmartin
I would like to check for interface seq consistency for groupBy now that we are not just using the default implementation. Can you add the tests for groupBy for all instance of IsSequence?

The pragmas are not defined but I could not figure out how to have WARNING or DEPREDATED to work correctly for having a compiler warning about dropping support for old vector versions.
discourse thread

@jcmartin
Copy link
Contributor Author

@BebeSparkelSparkel Done.

BebeSparkelSparkel
BebeSparkelSparkel requested changes Nov 28, 2024
View reviewed changes
mono-traversable/test/Main.hs Outdated Show resolved Hide resolved
mono-traversable/test/Main.hs Show resolved Hide resolved
@BebeSparkelSparkel BebeSparkelSparkel merged commit 95c991b into snoyberg:master Nov 28, 2024
19 checks passed
@BebeSparkelSparkel
Copy link
Collaborator

@jcmartin Thanks!

@jcmartin
Copy link
Contributor Author

@BebeSparkelSparkel Thank you for your work in helping merge this! I look forward to seeing 1.0.21.0 on hackage. :)

@BebeSparkelSparkel
Copy link
Collaborator

@jcmartin Thanks for reminding me to get permissions for that. I'll work on that this week.

@mgsloan mgsloan mentioned this pull request Dec 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BebeSparkelSparkel BebeSparkelSparkel BebeSparkelSparkel approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

store no longer compiles with new version vector-0.13.2.0.
2 participants
@jcmartin @BebeSparkelSparkel