-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Cmd no globals #2353
Cmd no globals #2353
Conversation
A PR looks good 👍 However, I have a small readability improvement point. What if we use a more explicit name instead of the At some point, we (and other people) are going to read it again and ideally, we should limit the number of the shortcuts like that. And sometimes it looks not consistent, example: outputs, err := createOutputs(conf.Out, src, conf, runtimeOptions, executionPlan, osEnvironment, logger, fl) |
as long as you come up with something. I guess part of the review process will be coming up with names about Also I might break this in 3 PRs :
WDYT? |
configFilePath: os.Getenv("K6_CONFIG"), // Overridden by `-c`/`--config` flag! | ||
exitOnRunning: os.Getenv("K6_EXIT_ON_RUNNING") != "", | ||
showCloudLogs: true, | ||
runType: os.Getenv("K6_TYPE"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of those os.Getenv should go, but as currently this are the only places where they are and there are zero tests covering this I decided against introducing even more changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an amount of code that I hope at some point will be rid by croconf, just spot one tiny typo
cmd/root.go
Outdated
noColor bool | ||
address string | ||
) | ||
// TODO better name - there are other commadn flags these are just ... non lib.Options ones :shrug: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO better name - there are other commadn flags these are just ... non lib.Options ones :shrug: | |
// TODO better name - there are other command flags these are just ... non lib.Options ones :shrug: |
bd459ad
to
677ba5b
Compare
677ba5b
to
77ee02e
Compare
I've update the name and have split this in 3 PRs so this is only about the flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, PR looks good to me! 💪
Don't worry, I actually will be waiting for @na-- to give a 👍 as well, as the person who probably is most familiar with all the things that can go wrong in this code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, besides the minor nitpick for the comment! 🎉 This is definitely not the final state these should be in, but a great step in the right direction! ❤️
Co-authored-by: na-- <n@andreev.sh>
b547412
to
7c1ecd7
Compare
edit: go commit by commit otherwise it's very hard to follow what happens IMO
edit2: I have optimized for small change so the code review can be done faster and also to not break anything. The final code is still pretty terrible for many ... many reasons, but fixing those is IMO outside the scope of this PR.
edit3: this was split so that the smaller changes are now in #2357 and #2358. And this PR is based on #2358 as it kind of depends on it to run the tests in parallel