fix: better management of relative paths#608
Merged
Conversation
ralgozino
requested review from
Filo01,
kriive,
nutellinoit and
stefanoghinelli
and removed request for
kriive
ralgozino
force-pushed
the
feat/relative-paths-code-reuse
branch
from
d972d00 to
45b01a7
Compare
- Improve management of relative paths, calculating them once. - Set outdir globally and reuse. - Reduce code duplication (still lots to improve) - Move flags processing to helper func when present - Improve UX on some error messages, help messages and debug messages - Normalize words in help messages Fixes #508
- Fix logic on some commands now that we have outdir always set to an absolute path - Fix some UX messages
Put var name between qoutes in the error message to make evident if there is a space in the env var name, for exmaple.
mention that the changes are not potentially safe, the user already knows they are applying new configuration.
ralgozino
force-pushed
the
feat/relative-paths-code-reuse
branch
from
45b01a7 to
ea469c0
Compare
kriive
suggested changes
May 30, 2025
Contributor
kriive
left a comment
kriive
left a comment
There was a problem hiding this comment.
some nitpicks but overall LGTM!
define `err` variable in func output instead of standalone Co-authored-by: Manuel Romei <manuel.romei@reevo.it>
kriive
previously approved these changes
May 30, 2025
Contributor
kriive
left a comment
kriive
left a comment
There was a problem hiding this comment.
Linter's failing on my idiomatic suggestions ¯_(ツ)_/¯ let's revert my code suggestions and follow the linter (which I do not agree with btw lol)
This reverts commit a9be1df. The change did not pass the linting checks.
Co-authored-by: Manuel Romei <manuel.romei@reevo.it>
Error out when phase and post-apply-phases are set, post-apply-phases will be ignored when phase is set. We should probably support both flags simultaneously in the future, but now at least we error out instead of ignoring the post-apply-phases flag. Relates: #602
Member
Author
Yeah, I liked your suggestions. It looked cleaner, but the linter has a point: https://github.com/FireFart/nonamedreturns I also added a check for when |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary 💡
Fixes #508
Description 📝
The main goal of this PR is to fix an issue when setting outdir to a relative path (like
.) resulted in an error on the apply command (and others).The issue was caused by several problems, but mainly:
outdirwas calculated as a permanent pre-run of the root command, but overwritten laterIn this PR I tried to fix these 3 issues, by having the root command update the flags with the right values and reusing them when possible. Also, in each command I moved the logic that handles the flags parsing to the dedicated function (if it was present).
I also took the chance to improve the UX, by adding some details to the messages that we show the user, adding some more debug logs, and unifying the style of the help messages.
I tried to not do a complete overhaul of the code to avoid breaking stuff and making this PR self-contained. We should make more of this iterative improvements. I think that this PR is a step in the right direction.
Breaking Changes 💔
None
Tests performed 🧪
Future work 🔧
There's still a lot of code duplication, the flags processing can probably be improved and there should be a way to handle them more smartly. We use almost the same flags on several commands.