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

cmds/core/seq: remove global flags, reflect and simplify tests #2761

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

binjip978
Copy link
Contributor

No description provided.

Signed-off-by: Siarhiej Siemianczuk <pdp.eleven11@gmail.com>
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Patch coverage is 60.00% of modified lines.

Files Changed Coverage
cmds/core/seq/seq.go 60.00%

📢 Thoughts on this report? Let us know!.

@binjip978 binjip978 added the Awaiting reviewer Waiting for a reviewer. label Sep 12, 2023
Copy link
Contributor

@MDr164 MDr164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, technically you still got the flags as package global but they're much better organized and accessed now. You could probably even move the flag var declarations into the first lines of main to make them fully private but that's just my take on it.

@binjip978
Copy link
Contributor Author

Looks good, technically you still got the flags as package global but they're much better organized and accessed now. You could probably even move the flag var declarations into the first lines of main to make them fully private but that's just my take on it.

Yeah, you absolutely right. This pr is mostly of making tests more testable (like t.Parallel if needed), but I fully agree that moving flags to main would be a better idea, but if we will do that in all commands we will got a test coverage hit. So we either need to introduce some kind of parseFlags function or give up a bit on test coverage.

@binjip978 binjip978 merged commit 0049777 into u-root:main Sep 13, 2023
23 of 24 checks passed
@binjip978 binjip978 deleted the seq branch September 13, 2023 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting reviewer Waiting for a reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants