Skip to content
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

Spike out setting site.url and site.baseurl #76

Merged
merged 6 commits into from
Oct 5, 2016
Merged

Spike out setting site.url and site.baseurl #76

merged 6 commits into from
Oct 5, 2016

Conversation

benbalter
Copy link
Contributor

@benbalter benbalter commented Oct 5, 2016

This allows relative_url and absolute_url to "just work" in most cases, by silently setting site.url and site.baseurl if either are not already set by the user.

This is a quick spike, and could use:

  • Code review
  • Passing tests

@benbalter
Copy link
Contributor Author

@parkr I believe you have a similar implementation... if you want to take this over, or close in favor of yours, please feel free to do so if you think you can get it across the 🏁 .

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

I like this a lot. Happy to take over fixing the tests if you need.

def generate(site)
Jekyll::GitHubMetadata.log :debug, "Initializing..."
@site = site
site.config["github"] = github_namespace
site.config["url"] = drop.url if site.config["url"].nil?
Copy link
Member

Choose a reason for hiding this comment

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

This could be site.config["url"] ||= drop.url and same for baseurl. Would save you an explicit if 😄

before { stub_api("/repos/jekyll/jekyll.github.io" , "repo") }
before { stub_api("/repos/jekyll/jekyll.github.io/pages" , "repo_pages") }
before { stub_api("/repos/jekyll/jekyll.github.com" , "repo") }
before { stub_api("/repos/jekyll/jekyll.github.com/pages" , "repo_pages") }
Copy link
Member

Choose a reason for hiding this comment

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

I'd put all of these into one before(:each) block. Should be a bit faster. Same with the env vars below.

@benbalter
Copy link
Contributor Author

@parkr made the ||= change... if you have time to help get the tests over the finish line, I think you're more knowledgeable about the tests here (which is where I started hitting my limits).

@parkr
Copy link
Member

parkr commented Oct 5, 2016

@benbalter Got them working on my machine. Does this LGTY?

@parkr
Copy link
Member

parkr commented Oct 5, 2016

@benbalter Because you made the PR you can't approve or request changes, but feel free to comment and I'll check back for your final 👍 .

@benbalter benbalter changed the title WIP: Spike out setting site.url and site.baseurl Spike out setting site.url and site.baseurl Oct 5, 2016
@benbalter
Copy link
Contributor Author

@parkr Nice work. Thanks for the help with the tests. Really like your implementation (which is where I was getting stuck). Think this is going to be a big improvement. ✨

@benbalter benbalter merged commit 366dee4 into master Oct 5, 2016
@benbalter benbalter deleted the baseurl branch October 5, 2016 17:16
benbalter added a commit that referenced this pull request Oct 5, 2016
parkr added a commit that referenced this pull request Oct 6, 2016
* master:
  Update history to reflect release of 2.1.0
  Bump to 2.1.0
  format history
  Update history to reflect merge of #76
  Only set `site.url` and `site.baseurl` if in JEKYLL_ENV=production
  Fix 2 incorrect tests
  Fix repository spec per removed ApiStub class
  Unify the way we stub all API requests
  use ||=
  spike out setting site.url and site.baseurl
@jekyll jekyll locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants