Skip to content

Enable pedantic for more components #4061

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

jhrcek
Copy link
Collaborator

@jhrcek jhrcek commented Feb 9, 2024

No description provided.

@@ -72,5 +72,3 @@ if impl(ghc >= 9.7)
-- this is okay
allow-newer:
ekg-core:text,
-- https://github.com/haskell-primitive/primitive-unlifted/issues/39
primitive-unlifted:bytestring,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like primitive-unlifted is not used by anything anymore, so removing it from here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a transitive dependency, but if it works then it works!

indirectOldNames <- concat . filter ((>1) . Prelude.length) <$>
mapM (uncurry (getNamesAtPos state) . locToFilePos) directRefs
indirectOldNames <- concat . filter notNull <$>
mapM (uncurry (getNamesAtPos state)) (mapMaybe locToFilePos directRefs)
Copy link
Collaborator Author

@jhrcek jhrcek Feb 9, 2024

Choose a reason for hiding this comment

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

Changed locToFilePos (see definition below) to return Maybe.
I hope that silently ignoring paths for which uriToNormalizedFilePath returns Nothing, rather than crashing is ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think we might want to fail the rename, because that means there's a potential reference that we couldn't resolve, so we might not be doing a correct rename.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just add a comment at least.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, will dive a bit into how this works and get back to this.

Copy link
Collaborator Author

@jhrcek jhrcek Feb 9, 2024

Choose a reason for hiding this comment

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

Ok, I found there's a dedicated getNormalizedFilePathE that does Uri -> NormalizeFilePath translation and throws PluginError when this fails. I switched to using that because it sounds cleaner/more informative than pattern match error..

@jhrcek jhrcek marked this pull request as ready for review February 9, 2024 08:44
indirectOldNames <- concat . filter ((>1) . Prelude.length) <$>
mapM (uncurry (getNamesAtPos state) . locToFilePos) directRefs
indirectOldNames <- concat . filter notNull <$>
mapM (uncurry (getNamesAtPos state)) (mapMaybe locToFilePos directRefs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I think we might want to fail the rename, because that means there's a potential reference that we couldn't resolve, so we might not be doing a correct rename.

indirectOldNames <- concat . filter ((>1) . Prelude.length) <$>
mapM (uncurry (getNamesAtPos state) . locToFilePos) directRefs
indirectOldNames <- concat . filter notNull <$>
mapM (uncurry (getNamesAtPos state)) (mapMaybe locToFilePos directRefs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just add a comment at least.

@jhrcek jhrcek marked this pull request as draft February 9, 2024 10:59
@jhrcek jhrcek force-pushed the jhrcek/more-pedantic branch from cbfe71a to 1391769 Compare February 9, 2024 12:06
@jhrcek jhrcek marked this pull request as ready for review February 9, 2024 12:09
@jhrcek jhrcek merged commit e37ec7d into haskell:master Feb 9, 2024
@jhrcek jhrcek deleted the jhrcek/more-pedantic branch February 12, 2024 09:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants