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 double .cr.cr extension in require path lookup #13749

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Aug 16, 2023

If a require path ends in .cr, some lookup paths would end up with a double extension .cr.cr. This is pretty unlikely to be ever intended. An example use case is a shard name that ends in .cr (see #8665, this patch is the first stage for fixing this issue).

This patch removes the double extensions to make lookup paths more reasonable. The main change is happening in ed8a8b2. The spec changes in this commit clearly show the effects.

It would be possible to maintain backwards compatibility by keeping the old paths around. I don't think this is really necessary because the filenames are so weird and I don't expect that to be used.

So I'd rather prefer it as a simple replacement. If we notice any issues, we can add the removed paths back in.

This addresses part of #13210

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

@beta-ziliani beta-ziliani added this to the 1.10.0 milestone Aug 17, 2023
@straight-shoota straight-shoota merged commit cc0c109 into crystal-lang:master Aug 19, 2023
51 of 53 checks passed
@straight-shoota straight-shoota deleted the fix/compiler-path-.cr branch August 19, 2023 19:32
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
This pull request was closed.
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