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

Allow referencing assets from the root of the project. #707

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

gdotdesign
Copy link
Member

Currently, assets an only be referenced relative to the file which references them. As per the issue, if there is a file deep in the source directory, referencing a file in the project root requires a lot of ../.

The PR makes it so that the root path / is the project root directory which have the mint.json file.

Resolves #459

@gdotdesign gdotdesign added the enhancement New feature or request label Nov 18, 2024
@gdotdesign gdotdesign added this to the 0.21.0 milestone Nov 18, 2024
@gdotdesign gdotdesign requested a review from Sija November 18, 2024 16:12
@gdotdesign gdotdesign self-assigned this Nov 18, 2024
@@ -11,7 +11,7 @@ Dir
sample, expected = File.read(file).split("-" * 80)

# Parse the sample
ast = Mint::Parser.parse(sample, file)
ast = Mint::Parser.parse(sample, File.dirname(__FILE__) + file.lchop("./spec"))
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of chopping off the ./spec from the filepath?

Copy link
Member Author

Choose a reason for hiding this comment

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

File.dirname(__FILE__) is the spec directory, so we need to remove that from the test file.

Copy link
Member

Choose a reason for hiding this comment

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

And what's wrong with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would not find the assets defined in specs.

/path/to/spec/.spec/test-file where /path/to/spec/.spec is not a file or a directory.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't because you've added a mint.json file to the spec/ folder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the spec folder becomes the root folder / for the tests. So for example /fixtures/data.txt is reachable so as ../../fixtures/data.txt from spec/compilers/directives/inline test.

Copy link
Member

Choose a reason for hiding this comment

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

And why don't you reference the files from the root folder instead? Unless I'm mistaken this way it wouldn't need any hacks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow. We would still need to chop ./spec off. It doesn't matter if it's absolute or relative. And we still need to find the closest mint.json to figure out the root folder.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I don't fully grok the situation. I'm gonna approve it and come back to it later, once I familiarize myself with the recent changes - and there's a lot of 'em 😅

src/ext/file.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
@gdotdesign gdotdesign requested a review from Sija November 20, 2024 08:20
@gdotdesign gdotdesign merged commit ada378a into master Nov 21, 2024
3 checks passed
@gdotdesign gdotdesign deleted the absolute-asset-path branch November 21, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Add asset path alias
2 participants