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

[fix #219] always qualify unopened imports #220

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Conversation

flupe
Copy link
Contributor

@flupe flupe commented Nov 8, 2023

The fix was easy enough, one branch for qualified imports (without a name) started returned Nothing after #202.

I changed a few lines that I found very hard to read, but can revert if it isn't to your taste. Also, can the lookup ever fail? I tried removing the catchError line locally and nothing happened on the test suite.

Copy link
Contributor

@omelkonian omelkonian left a comment

Choose a reason for hiding this comment

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

LGTM

@jespercockx can you please confirm the removal of catchError was indeed redundant?

Comment on lines +154 to +156
isDataMod <- isJust <$> isDatatypeModule (amodName x)
return $ QualifiedAs (if isDataMod then Nothing else Just qual)
_ -> return $ QualifiedAs Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed that's much clearer / easier to read.

@jespercockx
Copy link
Member

Hm, the catchError might be an artifact left over from an earlier version of the function, it's probably safe to remove it (assuming there's no test case that it triggers).

@flupe
Copy link
Contributor Author

flupe commented Nov 10, 2023

It doesn't appear to be an artifact and was introduced when the call to lookupModuleInCurrentModule was added, so let's play it safe for now and keep it. Merging.

@flupe flupe merged commit 57811ed into agda:master Nov 10, 2023
6 checks passed
@flupe flupe deleted the issue219 branch November 10, 2023 13:50
@jespercockx jespercockx added this to the 1.2 milestone Nov 27, 2023
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