feat(cli): add autoloading of the config file#361
Merged
Conversation
matejchalk
reviewed
Dec 13, 2023
Tlacenka
previously requested changes
Dec 13, 2023
Collaborator
Tlacenka
left a comment
There was a problem hiding this comment.
It is a great improvement overall, having the configuration extension not hardcoded.
The reason I put "request for changes" is the reduced coverage and missing documentation of the new logic.
packages/core/src/lib/implementation/read-code-pushup-config.integration.test.ts
Show resolved
Hide resolved
Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
…egration.test.ts Co-authored-by: Matěj Chalk <34691111+matejchalk@users.noreply.github.com>
matejchalk
previously approved these changes
Jan 26, 2024
Tlacenka
suggested changes
Jan 26, 2024
Collaborator
Tlacenka
left a comment
There was a problem hiding this comment.
Overall a nice improvement!
Main feedback - the lost integration test coverage should be restored before merging.
matejchalk
previously approved these changes
Jan 26, 2024
Tlacenka
approved these changes
Jan 26, 2024
Collaborator
Tlacenka
left a comment
There was a problem hiding this comment.
Test coverage restored.
Let us please not forget about the rest of the comments in a follow-up PR.
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
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.
Included in this PR is:
closes #307
Update:
I moved in the second option as it cleans up the codebase without hacks.
Will open another PR for the config refactoring.
Proposed additional changes to this PR
Problem
ATM
yargsis not configured correctly:configAPI is usedoptionsare withoutdefaultvaluesmiddlewareThis setup is hard to setup, reason about maintain and extend as unintuitive and fragile.
Suggested solution
We suggest to use the
configAPI ofyargsand implement the default values correctly.This would be like the following:
configAPI is used and filled with potential existingcode-pushup.config.(ts|mjs|js)optionscontaindefaultvaluesmiddlewaregets removed completelyThere are 2 possible ways to get close to the above solution:
Load RC config in a separate step before the CLI executes the command
As a proper solution is outside of this PR I suggest we discuss the following change:
cliandyargsCliasync and use the configure `yargs``Execute the middleware before argument validation
A middleware can be added with the option
{applyBeforeValidation: boolean}.We can use it and at lease reduce the hack with default values a bit. (config middle ware solves multiple problems ATM)
Still not as clean as the above when it comes to reasoning etc...