-
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
Simplify requirements calculation in k6 inspect
#2444
Conversation
// | ||
// TODO: completely remove this? | ||
func (r *Runner) IsExecutable(name string) bool { | ||
_, exists := r.Bundle.exports[name] | ||
return exists | ||
return r.Bundle.IsExecutable(name) | ||
} |
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.
This is used so unless we change the Runner interfaces it's needed to stay. Arguably the Runner
interface is quite big and IMO has parts that have just been piled one over the years. A full redesign though is probably better left for later.
I just did look into it and I think IsExecutualbe as well as Setup/Teardown around stuff should probably be moved to not the Runner and consolidated a bunch. Unfortunately that looks like it will need a lot of changes across k6 and some unknown unknowns that I don't think we should try to figure out now.
But tl;dr: you can't remove this, easily
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.
Yeah, I already addressed it in #2409 (comment), but I remove this TODO in a future PR, for now I'd prefer to merge it just so I don't have to rebase everything again.
But yeah, mid to long term, now that we have a (hopefully) viable solution for refactoring the mess of the Engine
away, we really need to start looking at refactoring the Runner
mess 😞 See also this comment from one of the other PRs #2412 (comment)
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.
I really would've preferred if js.Runner
and consequently js.Bundle
weren't directly used in cmd
(or anywhere else but js
) but that likely needs a complete refactor and this is in general better so 👍
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.
👍
We didn't end up merging more PRs in the
next
branch, so this is exactly the same as #2409, only a PR tomaster
this time