-
Notifications
You must be signed in to change notification settings - Fork 25
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
Set up Rubocop for the template #320
Conversation
a43227d
to
086e8ad
Compare
b510ba8
to
07f798f
Compare
07f798f
to
97c742c
Compare
9c34d96
to
59787bf
Compare
Style/GlobalVars: | ||
Enabled: false | ||
|
||
# TODO: Enable this rule later |
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'll enable these rules later in another PR, so this PR is not too big. 🙏
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.
@rosle , here are two of my favorites cops:
Is this something we could incorporate, what do you think? Thank you!
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.
Noted. I'll open more PRs to fix these cops and I'll revisit those rules 👍 ✨
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.
@tyrro I revisited these 2 cops, seems we already have it enabled 👍 🚀
@@ -1 +1,3 @@ | |||
# rubocop:todo Style/ClassAndModuleChildren |
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'll fix this when working on refactoring the template - #321. Because it needs to load the template module in prior. 🙏
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.
Looks good to me. I just have one question.
README.md
Outdated
If running on Apple M1: | ||
|
||
- Set bundle platform to ruby `bundle config set --global force_ruby_platform true` | ||
- To build docker image, please make sure to set platform to AMD64 `export DOCKER_DEFAULT_PLATFORM=linux/amd64` |
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 have a reference/source for this workaround? We might need to have it on other templates too.
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 have issue when building the image, Chrome is failed to installed
#9 7.669 E: Unable to locate package google-chrome-stable
and seems it is because we need to supply --platform
when doing docker build
From docker documentation:
- https://docs.docker.com/engine/reference/commandline/build/#options
- https://docs.docker.com/desktop/mac/apple-silicon/#known-issues
- There are also https://docs.docker.com/desktop/multi-arch/ But I still haven't played much on it. Still don't know about it much.
We can supply that option in many places, --platform
when build through cli, platform
in docker compose file or through ENV export DOCKER_DEFAULT_PLATFORM=linux/amd64
Now I just set that ENV in my zshrc 👍
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.
For bundle config set --global force_ruby_platform true
, without it, the Gemfile.lock
will generate
PLATFORMS
arm64-darwin-21
(While usually we have platform ruby)
The project is created successfully but when running docker container, it fails (I think maybe because platform is different?). The error is from Nogokiri
🛠️ To fix: we can just add ruby to platforms
PLATFORMS
arm64-darwin-21
ruby
I found bundle config set --global force_ruby_platform true
is easier, so we do not have to keep editing the lock file. 👀
But after I tried to reproduce, it does not happen anymore (maybe because we set platform 💭 ). Now Gemfile.lock
in the container is automatically modified to
PLATFORMS
arm64-darwin-21
x86_64-linux
I'll revisit this config again tomorrow 🤔
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 can confirm bundle config set --global force_ruby_platform true
is not required anymore. Maybe it's from platform setting:
Inside container, The additional platform is added:
I've updated the readme on f502949 👍
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.
LGTM!
Style/GlobalVars: | ||
Enabled: false | ||
|
||
# TODO: Enable this rule later |
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.
@rosle , here are two of my favorites cops:
Is this something we could incorporate, what do you think? Thank you!
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.
small tip, maybe you already know:
When linking a PR to an issue, we can use special keywords to close the issue automatically when the PR is merged, e.g.
close #168
.template/.rubocop.yml
Outdated
@@ -0,0 +1,53 @@ | |||
AllCops: | |||
NewCops: enable | |||
TargetRubyVersion: 2.7 |
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.
shouldn't we target ruby 3?
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.
Updated in 25f0dc4 ✨
Thank Long 👍 There will be a couple more PRs to fix the disabled Rubocop. I disabled those rules now to prevent the PR from getting too big. I'll do it in the last PR~ ✨ |
#168
What happened
Test Template
Insight
Due to our current template structure, there are some customization on Bundle and Rubocop.
Current structure:
The problem is we would need to run
rubocop
on the root of the project. So it runs tests for all the base template in the root and addon/varient in.template
. Hence, we need to:.bundle/config
to point to.template/Gemfile
. With this we will able to runbundle install
andbundle exec rubocop
from the root of the projectrubocop --config .template/.rubocop.yml
. I've added the script in.template/bin/rubocop
.Due to the current file structure limitation, we need to have these configs in order to run rubocop. Later we can revisit the template structure e.g. consider moving base into
.template
🤔Proof Of Work
Able to run rubocop through
.template/bin/rubocop
at the root of the project.The
Test Template
workflow is run.