Conversation
|
|
||
| typecheck: | ||
| {{ if semver_matches(`ruby -e "puts RUBY_VERSION"`, ">=2.7") == "true" { \ | ||
| "bundle exec srb tc" \ |
There was a problem hiding this comment.
this whole line gets executed if the expression is truthy
There was a problem hiding this comment.
can you clarify this? im not sure what the comment is calling out
There was a problem hiding this comment.
Mostly just explaining the just syntax. If the semver_matches call above is "true", then just runs bundle exec src tc. I found it a little counter intuitive, because it looks like the expression is returning a string (which it is, but that string just becomes a line in the file). Was just adding context to the reviewer
There was a problem hiding this comment.
ohhh. so this conditionally inserts "bundle exec srb tc" into the typecheck: step when it is run/evaluated, and thats what just executes. is that the standard way to do conditional execution in just, or is that something special we had to do here?
There was a problem hiding this comment.
ohhh. so this conditionally inserts "bundle exec srb tc" into the typecheck: ste
exactly!
is that the standard way to do conditional execution in just
Yes, or something like:
should_sorbet := semver_matches(`ruby -e "puts RUBY_VERSION"`, ">=2.7")
typecheck:
[ {{ should_sorbet }} == "true" ] && bundle exec srb tc
But then we shell out to ruby every time someone runs just, which has a small performance implication. Otherwise, they're equivalent!
There was a problem hiding this comment.
makes sense to me, and it seems readable on its face.
| bundle exec rake test | ||
|
|
||
| typecheck: | ||
| {{ if semver_matches(`ruby -e "puts RUBY_VERSION"`, ">=2.7") == "true" { \ |
There was a problem hiding this comment.
just supports semver checks out of the box, so I got to simplify this block!
There was a problem hiding this comment.
just to confirm, is this the "runs directly" referenced from the Rakefile? I think the same concerns I had with node/package.json applies; does having it in the Rakefile support some base level of development that someone who isnt comfortable with just can still benefit from?
There was a problem hiding this comment.
just to confirm, is this the "runs directly" referenced from the Rakefile?
no, that's that we call bundle exec rubocop in just in CI instead of bundle exec rake rubocop in CI.
does having [rubocop] in the Rakefile ...
probably a little, but it's also a very common ruby pattern to bundle exec <tool> to run local tools.
Having it in the rakefile means a user could bundle exec rake rubocop instead of bundle exec rubocop. That would be useful if we were adding other arguments or configuration in that task, but since it's literally just calling rubocop like the user would, involving rake with rubocop at all strikes me as unnecessary abstraction.
There was a problem hiding this comment.
This comment is referencing how much clear this is compared to the same check in the Makefile, which is a bunch of extra bash code (compared to just, which supports semver checks directly)
|
They literally changed what We can pin to the older ubuntu or update our jruby (maybe to |
| test: | ||
| name: Test (${{ matrix.ruby-version }}) | ||
| # this is needed because our JRuby test version isnt supported on ubuntu-24 (which is now ubuntu-latest) | ||
| # this version of jruby isn't available in the new latest (24.04) so we have to pin (or update jruby) |
There was a problem hiding this comment.
since you changed all of the runs-on, i might suggest moving some version of this comment to the top of the file so a future dev who might be tempted to upgrade the runs-as version would have the full context
There was a problem hiding this comment.
ah yeah, that's fine too. Also this change doesn't need to be included - we both changed the runs-on line in separate PRs, so this was my explanation for it. Yours also works though
Rakefile
Outdated
| t.pattern = "./test/**/*_test.rb" | ||
| end | ||
|
|
||
| # I think we can remove this; we'll run rubocop directly |
There was a problem hiding this comment.
i think the key here is the version test, since we support back to 2.3. is that easier to do here vs where you wanted to move it to?
There was a problem hiding this comment.
IMO the version check in this block isn't relevant anymore.
We only install rubocop on >= 2.7 (in the gemfile), so when we were calling rake across all versions, it was important to only require rubocop on versions in which it was available. But CI now calls rubocop directly (not through rake), so we don't need the task here at all (in a conditional or otherwise).
There was a problem hiding this comment.
oh interesting. agreed then it makes sense to remove it!
jar-stripe
left a comment
jar-stripe
left a comment
There was a problem hiding this comment.
Thanks for the answers/additional info. Looks good!
2 participants
Why?
In an effort to modernize and simplify our local tooling, we're moving our dev commands from makefiles to justfiles. This is intended to be mostly a drop-in replacement, but some command names may change per standardization efforts.
What?
justSee Also
DEVSDK-2325