-
Notifications
You must be signed in to change notification settings - Fork 557
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
add justfile #1513
add justfile #1513
Changes from 9 commits
19025d7
a43a7ee
ff4ddbb
fc00553
ddc3fbc
ae62982
fd2ac60
24769db
f53dc83
d165ad1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ Rake::TestTask.new do |t| | |
t.pattern = "./test/**/*_test.rb" | ||
end | ||
|
||
# I think we can remove this; we'll run rubocop directly | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO the version check in this block isn't relevant anymore. We only install rubocop on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh interesting. agreed then it makes sense to remove it! |
||
if RUBY_VERSION >= "2.7.0" | ||
require "rubocop/rake_task" | ||
RuboCop::RakeTask.new | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
set quiet | ||
|
||
import? '../sdk-codegen/utils.just' | ||
|
||
_default: | ||
just --list --unsorted | ||
|
||
install *args: | ||
bundle install {{ if is_dependency() == "true" {"--quiet"} else {""} }} {{ args }} | ||
|
||
# ⭐ run all unit tests | ||
test: install | ||
bundle exec rake test | ||
|
||
# check linting / formatting status of files | ||
format-check *args: install | ||
bundle exec rubocop {{ args }} | ||
alias lint-check := format-check | ||
xavdid-stripe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# ⭐ check style & formatting for all files, fixing what we can | ||
lint: (format-check "--autocorrect") | ||
|
||
# copy of `lint` with less output | ||
format: (format-check "--format quiet --autocorrect") | ||
|
||
update-certs: install | ||
bundle exec rake update_certs | ||
|
||
# run sorbet to check type definitions | ||
typecheck: | ||
{{ if semver_matches(`ruby -e "puts RUBY_VERSION"`, ">=2.7") == "true" { \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just supports semver checks out of the box, so I got to simplify this block! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
no, that's that we call
probably a little, but it's also a very common ruby pattern to Having it in the rakefile means a user could There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is referencing how much clear this is compared to the same check in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes sense to me |
||
"bundle exec srb tc" \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this whole line gets executed if the expression is truthy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you clarify this? im not sure what the comment is calling out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly just explaining the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
exactly!
Yes, or something like:
But then we shell out to ruby every time someone runs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense to me, and it seems readable on its face. |
||
} else { \ | ||
"echo \"Ruby version < 2.7, skipping srb tc\"" \ | ||
} }} | ||
|
||
# called by tooling | ||
[private] | ||
update-version version: | ||
echo "{{ version }}" > VERSION | ||
perl -pi -e 's|VERSION = "[.\-\w\d]+"|VERSION = "{{ version }}"|' lib/stripe/version.rb |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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