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.

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

Use html_url from Pages endpoint (behind preview env flag) #60

Merged
merged 6 commits into from May 26, 2016
Merged

Use html_url from Pages endpoint (behind preview env flag) #60

merged 6 commits into from May 26, 2016

Conversation

parkr
Copy link
Member

@parkr parkr commented May 25, 2016 •
edited

Adds a RepositoryCompat class which has a fallback #pages_url that Repository#pages_url uses in case it doesn't get the /pages info.

  • Handle preview API
  • Make it opt-in

/cc @jekyll/gh-pages for review.

@parkr parkr self-assigned this May 25, 2016
@benbalter
Copy link
Contributor

@parkr having a little trouble following the diff, due to noise. To confirm, it should use the API value for all fields (url, scheme, domain, etc.), and fall back to its own calculation, if the API isn't available, right?

parkr
parkr reviewed May 26, 2016
View reviewed changes
lib/jekyll-github-metadata/repository.rb
end

def repo_info
@repo_info ||= (Value.new(proc { |c| c.repository(nwo) }).render || Hash.new)
end

def repo_pages_info
@repo_pages_info ||= (Value.new(proc { |c| c.pages(nwo) }).render || Hash.new)
end
Copy link
Member Author

Choose a reason for hiding this comment

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

@benbalter this is key.

@parkr
Copy link
Member Author

parkr commented May 26, 2016

To confirm, it should use the API value for all fields (url, scheme, domain, etc.), and fall back to its own calculation, if the API isn't available, right?

It uses the API value for all fields (taking the html_url, putting it into URI() and extracting from there) unless the API value doesn't exist. If it doesn't exist, then it proxies to the RepoCompat layer, which is the old behaviour.

@benbalter
Copy link
Contributor

Thanks for the clarification. Makes sense to me.

@parkr
Copy link
Member Author

parkr commented May 26, 2016

@benbalter Confirmed that the current code is opt-in via PAGES_PREVIEW_HTML_URL and works fine on our canary page.

@parkr parkr changed the title Use html_url from Pages endpoint Use html_url from Pages endpoint (behind preview env flag) May 26, 2016
@parkr
Copy link
Member Author

parkr commented May 26, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit c3114f0 into master May 26, 2016
@jekyllbot jekyllbot deleted the use-html_url-from-pages-endpoint branch May 26, 2016 21:39
jekyllbot added a commit that referenced this pull request May 26, 2016
@jekyllbot
Update history to reflect merge of #60 [ci skip]
2069f5f
@jekyll jekyll locked and limited conversation to collaborators Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@parkr parkr

Labels
enhancement frozen-due-to-age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@parkr @benbalter @jekyllbot