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

Properly resolve user's path #20

Merged
merged 4 commits into from
Apr 6, 2025
Merged

Properly resolve user's path #20

merged 4 commits into from
Apr 6, 2025

Conversation

DeviousStoat
Copy link
Contributor

This implements only the part for the new command, the directory argument and sets up the tooling for the rest I guess.

I was spending way too much time pondering in front of my monitor for the rest. Didn't really want to ask too many questions but I guess I was not moving at all so I will ask. I am very confused about all the path options there is in the Hix.Optparse module. Why some paths are expected to be relative, some other absolute and some other absolute or cwd? Can't they all be absolute or relative to the cwd?
Can't all these and all these be reduced to just a pathUserOption to be resolved later as introduced in this PR (or the sum type you suggested previously if we need for the tests later)?

@tek
Copy link
Owner

tek commented Apr 5, 2025

Please don't hesitate to ask questions whenever you need!

When I introduced absPathOrCwdOption rather recently, I only changed a handful of options to use it, but there are likely some more that would benefit from it, so change those if you like.

However, there are some that cannot be absolute paths, because they are used in temporary directories, like the one in stateFileConfigParser (not sure if there are any others – at least rel{File,Path}Option have no other consumers).

@tek
Copy link
Owner

tek commented Apr 5, 2025

wanna merge or convert more use sites of the old parsers?

@DeviousStoat
Copy link
Contributor Author

both? :D I can make another PR

@tek
Copy link
Owner

tek commented Apr 6, 2025

alright then 😁

@tek tek merged commit 4460b54 into tek:main Apr 6, 2025
@DeviousStoat DeviousStoat deleted the path-user branch April 6, 2025 03: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