search

benbalter / jekyll-remote-theme

Jekyll plugin for building Jekyll sites with any GitHub-hosted theme
MIT License
291 stars 77 forks source link

Added extra context to error message when remote theme download fails #85

Closed IanLee1521 closed 3 years ago

IanLee1521 commented 3 years ago

This is an attempt to help give a pointer to why the download may be failing after a remote theme changes to using a default branch of main rather than master.

It relates to https://github.com/benbalter/jekyll-remote-theme/issues/59 (where @parkr proposed something like this) and https://github.com/benbalter/jekyll-remote-theme/issues/84 (where this has cropped up more recently).

Note that my Ruby is real rusty, so forgive me if this is not quite in the ideal standards.

welcome[bot] commented 3 years ago

Welcome! Congrats on your first pull request to Jekyll Remote Theme. If you haven't already, please be sure to check out the contributing guidelines.

benbalter commented 3 years ago

Thanks for this @IanLee1521. I got the tests fixed on the default branch, but it looks like there's one failing test expectation for the message now https://github.com/benbalter/jekyll-remote-theme/blob/master/spec/jekyll-remote-theme/downloader_spec.rb#L72. If you're able to get that expectation updated with the new expected message, I'd be happy to get this merged. Thanks!

GitHub
benbalter/jekyll-remote-theme
Jekyll plugin for building Jekyll sites with any GitHub-hosted theme - benbalter/jekyll-remote-theme
parkr commented 3 years ago

In Theme#git_ref, we could fall back to HEAD instead of master.

https://github.com/benbalter/jekyll-remote-theme/blob/bdcb1fccf28ce459bb3ad0024d9793fd3f6496a1/lib/jekyll-remote-theme/theme.rb#L53-L55

Compare:

IanLee1521 commented 3 years ago

@benbalter - Build is fixed. Thanks for the help there.

@parkr - I'd be down to make that change. That seems like it would be a better default for supporting whatever default branch names folks are using. @benbalter -- thoughts?

welcome[bot] commented 3 years ago

Congrats on getting your first pull request to Jekyll Remote Theme merged! Without amazing humans like you submitting pull requests, we couldn’t run this project. You rock! :tada:

If you're interested in tackling another bug or feature, take a look at the open issues, especially those labeled help wanted.

benbalter commented 3 years ago

I'd be down to make that change. That seems like it would be a better default for supporting whatever default branch names folks are using. @benbalter -- thoughts?

👍🏻 If you're willing to open that PR, I'd be happy to review and merge.

IanLee1521 commented 3 years ago

Will do. I'll work on that soon.

Out of curiosity, what is the path for getting things from "updates here in jekyll-remote-theme" to "being used by production GitHub pages" ?

benbalter commented 3 years ago

Out of curiosity, what is the path for getting things from "updates here in jekyll-remote-theme" to "being used by production GitHub pages" ?

I'm not part of the Pages team any more, but they've been updating the Pages Gem regularly, especially when there's a specific improvement that will benefit the community.

IanLee1521 commented 3 years ago

cool cool, well sounds like I should get that other (Use HEAD) improvement prepped and submitted too to be even better