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

Expose dirname from SourceCodePos #397

Merged
merged 3 commits into from
Sep 20, 2022

Conversation

mneumann
Copy link
Contributor

This will enable Spec to print absolute or project-root relative file paths.

This will enable `Spec` to print absolute or project-root relative file
paths.
@mneumann mneumann requested a review from jemc September 20, 2022 13:13
@mneumann
Copy link
Contributor Author

@jemc For Spec: I'd not like to report the full absolute path of the assert that was failing.
Instead I would like to report the path relative to the project root, e.g. src/Spec.savi.
Getting this information at runtime is tricky. We don't have pwd/getcwd in Savi yet and even then the question remains, do we report relative to the current working directory or relative to the executing bin/spec ....
So, in addition to the absolute dirname, I'd expose the directory relative to the root manifest.

Any suggestions?

@mneumann
Copy link
Contributor Author

Mainly, reporting absolute paths in Spec will break the Spec for Spec, as it would include my absolute paths. Also, I never really liked it to expose the absolute paths of my failing tests in "bug reports". No one should see something like /home/foobar/Clients/StupidClientA/... if I decide to name a client like this (which I don't) and reflect this in my directory structure :)

@jemc
Copy link
Contributor

jemc commented Sep 20, 2022

I agree that having Spec show the relative path compared to the path of the root manifest would be the most correct behavior - I prefer this over something more magical involving the "current working directory".

@jemc
Copy link
Contributor

jemc commented Sep 20, 2022

@mneumann - I think ctx.root_package_link.path will get you the path to the root package's manifest directory.

@mneumann
Copy link
Contributor Author

@jemc relative to root package might not always work in case the packages would not reside under deps. In case of Spec I would report pkgname + package relative file path.

core/SourceCodePos.savi Outdated Show resolved Hide resolved
@jemc
Copy link
Contributor

jemc commented Sep 20, 2022

relative to root package might not always work in case the packages would not reside under deps. In case of Spec I would report pkgname + package relative file path.

I'd honestly still prefer them being relative to the root package, so in the case of non-root package source files I'd be able to see which package they came from. But I'm not strongly opinionated enough to argue about it, as Savi's name requirements make collisions a lot less likely.

In the strange case you raise of dependencies not being in deps (which would be true of core until we do #366, so I guess it's not that strange at the moment), I'd expect to see ../ segments added to the head of the path until reaching a common parent directory that holds both the root package and the out-of-source-tree package.

@mneumann
Copy link
Contributor Author

@jemc I agree with you. Added filepath_rootpkgrel. We should change the naming. But this works fine so far with Spec. So I am going to merge and feel free to fixup later on.

@mneumann mneumann merged commit 01452ce into savi-lang:main Sep 20, 2022
@mneumann mneumann deleted the expose-dirname-to-source-code-pos branch September 20, 2022 19:40
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.

2 participants