-
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
Fix require warning with tests from stdin #3774
Conversation
This specifically fixes #3534 (comment) and related problems |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3774 +/- ##
==========================================
- Coverage 71.86% 71.84% -0.03%
==========================================
Files 291 291
Lines 21249 21255 +6
==========================================
Hits 15270 15270
- Misses 4917 4921 +4
- Partials 1062 1064 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d61ba18
to
fe7fd71
Compare
switch opt := o.(type) { | ||
case fsext.Fs: | ||
fs = opt | ||
case lib.RuntimeOptions: |
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.
Nit; out of curiosity - if we normally use *lib.RuntimeOptions
, is there any reason why not using it here instead of the value? 🤔
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.
Do you mean instead of using opts
?
We can - but this is a copy of the helper function just above here that does a very similar thing, but doesn't go through stdin.
I kind of prefered to not rewrite the whole funciton, and me trying to add it to the above was pretty painful and ended up being way bigger change.
So in the interest of not having to have a huge change (across everything using the above helper) - I decided to copy it and leave everything but what I need as it is.
If we add more tests for executing from stdin - this will likely need most of the other options and stuff.
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.
No, no, I was asking value vs ref.
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.
oh. Looking at it we basically have a pointer ... for no reason. It is used to check if it was set - but then we just set it to the empty ones, so IMO we probably can move to a not pointer variant.
What?
Why?
Checklist
make lint
) and all checks pass.make tests
) and all tests pass.Related PR(s)/Issue(s)