Skip to content

Adds make tidy #8439

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.

Sign up for GitHub

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

Merged
n1zyy merged 3 commits into from
May 24, 2023
Merged

Adds make tidy #8439

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d683fd4
Adds make tidy
n1zyy May 19, 2023
0f7a28c
PR feedback
n1zyy May 19, 2023
3839b7d
Update Makefile
n1zyy May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ ARTIFACT_DESTINATION_FILE ?= ./tmp/idp.tar.gz
brakeman \
build_artifact \
check \
clobber_db \
clobber_assets \
clobber_logs \
docker_setup \
download_acuant_sdk \
fast_setup \
Expand All @@ -34,6 +37,7 @@ ARTIFACT_DESTINATION_FILE ?= ./tmp/idp.tar.gz
optimize_assets \
optimize_svg \
run \
tidy \
update \
urn \
README.md \
Expand Down Expand Up @@ -268,3 +272,25 @@ README.md: docs/ ## Generates README.md based on the contents of the docs direct

download_acuant_sdk: ## Downloads the most recent Acuant SDK release from Github
@scripts/download_acuant_sdk.sh

clobber_db: ## resets the database for make setup

@zachmargolis zachmargolis May 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

Suggested change
clobber_db: ## resets the database for make setup
clobber_db: ## resets the database for make setup

@n1zyy n1zyy May 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I think this explanation makes it worse, but: I had done this deliberately, because it looked like that was the pattern used elsewhere in the file. But looking at it again, nothing else uses 2 spaces. Maybe I saw the two pound signs and wires got crossed in my mind...? Will fix these.

bin/rake db:create
bin/rake db:environment:set
bin/rake db:reset
bin/rake db:environment:set
bin/rake dev:prime

clobber_assets: ## removes assets

@zachmargolis zachmargolis May 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

Suggested change
clobber_assets: ## removes assets
clobber_assets: ## removes assets

bin/rake assets:clobber
RAILS_ENV=test bin/rake assets:clobber
Comment on lines +283 to +285

@aduth aduth May 22, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally find some value in this one, curious if it is aware of all of the asset outputs, i.e. public/assets (Sprockets), public/packs (Webpack/jsbundling-rails), app/assets/builds (Sass/cssbundling-rails).

@n1zyy n1zyy May 24, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.rubydoc.info/gems/sprockets-rails/2.3.3 suggests it isn't natively aware of some of those. public/packs is still there (15MB), for example. It looks like we can add them in an initializer, but I'd like to propose that be a separate PR if warranted.

@aduth aduth May 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to be fair, that seems like something the jsbundling-rails gem should be handling, since it's already doing things with other Rake tasks like rake assets:precompile.

@aduth aduth May 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n1zyy Based on what I see here, it can be achieved with javascript:clobber task:

rails/jsbundling-rails#87 (comment)

@n1zyy n1zyy May 24, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth I like the idea, but I can't seem to verify what it's actually removing.

https://github.com/rails/jsbundling-rails/blob/main/lib/tasks/jsbundling/clobber.rake suggests that it may automatically extend assets:clobber.

@n1zyy n1zyy May 24, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(More concretely: I can see that it runs rm_rf Dir["app/assets/builds/**/[^.]*.{js,js.map}"], verbose: false, but I don't actually have those files when running the app locally and don't follow why.)

@aduth aduth May 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I forgot that we do things a little custom such that we don't output JavaScript to app/assets/builds, which probably explains why you're not seeing them and not seeing the task have any meaningful impact. We'd probably also need something custom to delete public/packs, then.


clobber_logs: ## purges logs

@zachmargolis zachmargolis May 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space ```suggestion
clobber_logs: ## purges logs

rm -f log/*
rm -rf tmp/cache/*
rm -rf tmp/encrypted_doc_storage
rm -rf tmp/letter_opener
rm -rf tmp/mails

## Remove assets and logs, and unused gems, but leave DB alone
tidy: clobber_assets clobber_logs

@zachmargolis zachmargolis May 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

putting the comment at the end lets make help read it correctly

Suggested change
## Remove assets and logs, and unused gems, but leave DB alone
tidy: clobber_assets clobber_logs
tidy: clobber_assets clobber_logs ## Remove assets and logs, and unused gems, but leave DB alone

@n1zyy n1zyy May 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the "It would look better to say nothing" front... Until I saw this comment, I was unaware of make help! I thought we just had a pattern of commenting everything. 😇

Thanks for these comments; I'm going to clean these up and also make the comments read slightly better now that I have this context.

bundle clean
15 changes: 3 additions & 12 deletions bin/setup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,22 +56,13 @@ Dir.chdir APP_ROOT do
run "yarn install"

puts "\n== Preparing database =="
run 'bin/rake db:create'
run 'bin/rake db:environment:set'
run 'bin/rake db:reset'
run 'bin/rake db:environment:set'
run 'bin/rake dev:prime'
run 'make clobber_db'

puts "\n== Cleaning up old assets =="
run "bin/rake assets:clobber"
run "RAILS_ENV=test bin/rake assets:clobber"
run "make clobber_assets"

puts "\n== Removing old logs and tempfiles =="
run "rm -f log/*"
run "rm -rf tmp/cache"
run "rm -rf tmp/encrypted_doc_storage"
run "rm -rf tmp/letter_opener"
run "rm -rf tmp/mails"
run "make clobber_logs"

puts "\n== Restarting application server =="
run "mkdir -p tmp"
Expand Down