8000 script/default-site: accept flags for `jekyll new` by parkr · Pull Request #9259 · jekyll/jekyll · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

script/default-site: accept flags for jekyll new #9259

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

Jump to bottom
Merged
merged 4 commits into from
Jan 19, 2023
Merged

script/default-site: accept flags for jekyll new #9259

merged 4 commits into from
Jan 19, 2023

Conversation

parkr
Copy link
Member
@parkr parkr commented Jan 17, 2023

This is a 🔨 code refactoring.

Summary

In #9257, @ashmaroli proposed copying script/default-site to script/blank-site so that we could run jekyll new --blank then jekyll build the way that script/default-site does. I was hesitant to duplicate much of the script into another script, so I was thinking that maybe we could make the common components a single script.

With this, you can script/default-site --blank to run jekyll new --blank. It still runs the smoke test that says "does a blank site build".

The best outcome would probably be a Cucumber test which is already setup to do roughly this kind of operation (create a site, then run jekyll build, and verify that it built the expected output).

@parkr parkr mentioned this pull request Jan 17, 2023
Merged
@ashmaroli
Copy link
Member

Hmmm.. I'm really not able to wrap my head around the current diff. (Your idea is sound. But the implementation is sketchy.)

  • I am not able to instantly grok the purpose of using PID-specific tmp dir. Is it avoid deleting a pre-existing non-empty tmp dir?
  • I do not see the point of "$@" hook to jekyll build incantation. --blank is only valid for jekyll new. jekyll build doesn't recognise --blank.

The best outcome would probably be a Cucumber test..

I agree with this thought.
In fact, there's already a scenario testing jekyll build --blank (features/create_sites.feature:6). However, It just checks the presence of specific source files instead of running jekyll build and validating those source files.
IMHO, it'd be best if you can scrap the current proposal and enhance the above Cucumber test with additional 3steps (to cd into scaffolded blank site, run jekyll build and assert a zero exit status).

parkr
parkr commented Jan 18, 2023
View reviewed changes
script/default-site Outdated

echo "$0: creating new default site"
bundle exec jekyll new tmp/default-site
pushd tmp/default-site
bundle exec jekyll new "$TMP_SOURCE"
Copy link
Member Author

Choose a reason for hiding this comment

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

New "$@" should be here - sorry for the confusion @ashmaroli!

@parkr
Copy link
Member Author
parkr commented Jan 18, 2023

I am not able to instantly grok the purpose of using PID-specific tmp dir. Is it avoid deleting a pre-existing non-empty tmp dir?

Yeah, I was thinking that we'd invoke this twice and it'd be nice to not blow away the first one, but I can revert if you prefer! Let me know.

@ashmaroli
Copy link
Member
ashmaroli commented Jan 18, 2023 •
edited
Loading

I was thinking that we'd invoke this twice and it'd be nice to not blow away the first one..

If we are indeed continuing down the "Bash script" route (as against the "Cucumber test" route), I feel we can address your primary concern (about duplicated code) using just "Bash functions" to respect DRY and the secondary concern (about keeping directories separate and untouched) by exposing $@ to jekyll new.

setup_tmp_dir() { ... }

...

setup_tmp_dir()
[[ $@ ]] && args="$@" || args="default-site"
bundle exec jekyll new "$args"
...

script/default-site

script/default-site --blank

Update: I wasn't thinking straight. The above script isn't gonna work. Besides, dirname as PID hampers readability of both build log and file system tree itself.
I will employ DRY in my pull request itself.

@parkr parkr changed the title script/default-site: use a PID-specific tmp site dir & accept flags for jekyll build script/default-site: accept flags for jekyll new Jan 19, 2023
@parkr
Copy link
Member Author
parkr commented Jan 19, 2023

Updated to remove the PID-specific dir.

In general I’m trying to simplify, so adding another file with bash functions doesn’t feel like it simplifies.

@ashmaroli
Copy link
Member

Thanks @parkr
@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit bf8d50e into master Jan 19, 2023
@jekyllbot jekyllbot deleted the parkr-patch-2 branch January 19, 2023 07:05
jekyllbot added a commit that referenced this pull request Jan 19, 2023
@jekyllbot
Update history to reflect merge of #9259 [ci skip]
99a1b72
github-actions bot pushed a commit that referenced this pull request Jan 19, 2023
@jekyllbot
Deploy docs from bf8d50e
6c091d2 
Parker Moore: script/default-site: accept flags for `jekyll new` (#9259)

Merge pull request 9259
@jekyll jekyll locked and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ashmaroli ashmaroli ashmaroli approved these changes

Assignees
No one assigned
Labels
fix frozen-due-to-age internal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@parkr @ashmaroli @jekyllbot
0