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

Remove dependency on unordered-containers #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

414owen
Copy link

@414owen 414owen commented Jan 7, 2024

This lightens the dependency footprint of
diagnose by dropping the dependency on
unordered-containers.

This lightens the dependency footprint of
`diagnose` by dropping the dependency on
`unordered-containers`.
@sellout
Copy link

sellout commented Jul 14, 2024

This dependency removal seems less clear than the others. Some sort of Map is what’s desired here, so implementing the relevant functions locally on top of [] seems like the wrong way to go.

Maybe an argument could be made to use a Map (probably lazy?) from containers instead, since it’s already in the dependency graph and included with GHC anyway?

But restricting existing [] to NonEmpty when possible seems like a good change.

@414owen
Copy link
Author

414owen commented Jul 15, 2024

If it's just groupOn, then I would prefer a local implementation (or one imported from a lightweight library like extra), to using unordered-containers, but that's very much a personal preference. You can close this issue if you prefer another side of the trade-off :)

@Mesabloo
Copy link
Owner

Hello, sorry for the long silence.

I don't really know why I used unordered-containers in the first place, where it is used now. It most likely came with a bit more usage here and here, which I may have removed over the years.
Now the remaining uses of HashMaps do not really rely on the invariants of the structure itself: using Map instead of HashMap everywhere should not pose a problem. Not sure if those maps should be lazy or not though. It may perhaps be better for the map storing file contents, but not for those storing markers in the rendering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants