Use owner name as site title for User and Organization sites. #197

lardieri · 2021-02-23T10:22:24Z

Resolve #196

Using the repo name as the site title makes sense for Projects, which can choose their own repo names. But User and Organization sites must use repo names like {owner}.github.io which:

Does not look good as a browser tab title, bookmark, or header for a page that is supposed to be about a person or organization.
Conveys no useful information, as it is unlikely that anyone would clone or fork such a repo.
Looks confusing for users or organizations that use custom domains (CNAMEs), and have no other content indicating their site is hosted by GitHub.

The display name of the user or organization is a better choice. This is an optional property, so fall back to (login) name if display name is not present.

lardieri · 2021-02-23T11:33:13Z

I'd like to add a couple of new tests in site_github_munger_spec.rb around line 110 or so, but it looks like I might need to add a new test-site as well. Probably best to consult with a project owner before proceeding.

mattr- · 2021-02-23T14:23:09Z

Add your tests and a new test site. Whatever you think is necessary to properly test this.

mattr-

I think this is a nice addition as is, but i support your desire to add tests. Please add whatever tests and related fixtures (test site, etc.) that you think are necessary. However, if you find after a few days that adding the tests is a lot of extra work, let's go ahead and skip them and ship this without.

lib/jekyll-github-metadata/site_github_munger.rb

lib/jekyll-github-metadata/metadata_drop.rb

MichaelCurrin · 2021-03-01T07:06:18Z

CI checks are failing still.

lib/jekyll-github-metadata/site_github_munger.rb

.gitignore

MichaelCurrin · 2021-03-01T10:27:16Z

Thanks for renaming.

Please see additional suggestion on removing the code comment.

MichaelCurrin · 2021-03-01T10:31:14Z

Tests

I see two 2 failing AppVeyor tests from checks.

Tthe one looks like SSL error outside of this PR scope.

The other issue looks possibly relevant. Test job from build #37997173

      1) Jekyll::GitHubMetadata::Owner is an ORG fetches login
         Failure/Error: expect(subject.public_send(attribute)).to eq(expected_value)
           expected: "jekyll"
                got: nil
           (compared using ==)
         # ./spec/owner_spec.rb:43:in `block (4 levels) in <top (required)>'
    Finished in 1 minute 31.39 seconds (files took 1.23 seconds to load)

But also, tests are failing in general across PRs so this may have been an issue already.

https://ci.appveyor.com/project/jekyll/github-metadata/history

MichaelCurrin

Remove comment

lardieri · 2021-03-01T17:10:36Z

Tests

The other issue looks possibly relevant. Test job from build #37997173

My new code does not distinguish between User and Organization owners. If my code caused this failure, then the test case Jekyll::GitHubMetadata::Owner is a USER would have failed as well.

But also, tests are failing in general across PRs so this may have been an issue already.

https://ci.appveyor.com/project/jekyll/github-metadata/history

The failures are not consistent across obviously irrelevant commits, such as my change to .gitignore.

Nor are the failures reproducible locally using the same seed and the same Ruby / Jekyll versions in the environment.

It's reasonable to conclude that some issue with the AppVeyor environment is responsible for the failures. Possible hypotheses:

AppVeyor is not pulling the environment that it says that it is.
-- Perhaps a corrupt local cache of some kind.
-- Perhaps some bug or other difference in the Windows implementation of Ruby vs. Unix.
Hardware issue with AppVeyor worker machines (random 1-bit errors, etc.).
Some type of Chaos Monkey has been introduced.

There are also obvious issues with order dependency in these tests. For example:

--seed 41261 produces warnings for test cases when git doesn't exist and with name set, but no title
--seed 58031 runs the same tests in a different order and produces no warnings

(I would argue that the warnings are correct, given the context of those specific cases.)

So, I'm not losing sleep over these test failures. If you disagree, please suggest a concrete course of investigation.

MichaelCurrin · 2021-03-01T20:29:31Z

Thanks for investigating. Yes I suspected the tests were in an inconsistent state already, so further changes to tests aren't needed I think

Using the repo name as the site title makes sense for Projects, which can choose their own repo names. But User and Organization sites must use repo names like "{owner}.github.io" which: * Does not look good as a browser tab title, bookmark, or header for a page that is supposed to be about a person or organization. * Conveys no useful information, as it is unlikely that anyone would clone or fork such a repo. * Looks confusing for users or organizations that use custom domains (CNAMEs), and have no other content indicating their site is hosted by GitHub. The display name of the user or organization is a better choice. This is an optional property, so fall back to (login) name if display name is not present. Resolve jekyll#196

lardieri · 2021-05-29T10:07:06Z

Might also resolve #207 ?

When generating metadata for a GitHub Pages site of a User or Organization, the default site title should be: * the User/Org's display name, if there is one; or * the User/Org's login, if there is no display name.

lardieri · 2021-07-10T06:02:42Z

@mattr- I just pushed the test cases that I promised you back in February. Would you mind taking a look, and merging if you approve? Thank you.

lardieri · 2021-08-02T18:47:51Z

@mattr- and @MichaelCurrin - any further thoughts on this? or are you ready to merge it in? Thanks.

lib/jekyll-github-metadata/repository.rb

MichaelCurrin · 2021-08-03T11:13:35Z

@lardieri The code and tests look clear and thorough. Approved.

mattr- · 2021-08-03T15:54:44Z

@jekyllbot: merge +minor

lardieri changed the title ~~Resolve jekyll/github-metadata#196 - Use owner name as site title for User and Organization sites.~~ Use owner name as site title for User and Organization sites. Feb 23, 2021

mattr- approved these changes Feb 23, 2021

View reviewed changes

lib/jekyll-github-metadata/site_github_munger.rb Outdated Show resolved Hide resolved

MichaelCurrin suggested changes Mar 1, 2021

View reviewed changes

lib/jekyll-github-metadata/metadata_drop.rb Outdated Show resolved Hide resolved

MichaelCurrin reviewed Mar 1, 2021

View reviewed changes

lib/jekyll-github-metadata/site_github_munger.rb Outdated Show resolved Hide resolved

MichaelCurrin reviewed Mar 1, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

MichaelCurrin suggested changes Mar 1, 2021

View reviewed changes

lardieri force-pushed the issue-196 branch from 26b2974 to 9d6d274 Compare April 12, 2021 08:20

lardieri mentioned this pull request May 29, 2021

Add name of users #207

Closed

Test cases for setting title of User and Org sites.

5e5b0af

When generating metadata for a GitHub Pages site of a User or Organization, the default site title should be: * the User/Org's display name, if there is one; or * the User/Org's login, if there is no display name.

lardieri requested a review from mattr- July 12, 2021 21:24

MichaelCurrin reviewed Aug 3, 2021

View reviewed changes

lib/jekyll-github-metadata/repository.rb Show resolved Hide resolved

MichaelCurrin approved these changes Aug 3, 2021

View reviewed changes

mattr- approved these changes Aug 3, 2021

View reviewed changes

jekyllbot merged commit 0efda2e into jekyll:master Aug 3, 2021

jekyllbot added the enhancement label Aug 3, 2021

jekyllbot added a commit that referenced this pull request Aug 3, 2021

Update history to reflect merge of #197 [ci skip]

78ad271

parkr mentioned this pull request Jan 27, 2022

Time for a new release #215

Closed

jekyll locked and limited conversation to collaborators Aug 3, 2022

jekyllbot added the frozen-due-to-age label Aug 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use owner name as site title for User and Organization sites. #197

Use owner name as site title for User and Organization sites. #197

lardieri commented Feb 23, 2021 •

edited

Loading

lardieri commented Feb 23, 2021

mattr- commented Feb 23, 2021

mattr- left a comment

MichaelCurrin commented Mar 1, 2021

MichaelCurrin commented Mar 1, 2021 •

edited

Loading

MichaelCurrin commented Mar 1, 2021

MichaelCurrin left a comment

lardieri commented Mar 1, 2021

Tests

MichaelCurrin commented Mar 1, 2021 •

edited

Loading

lardieri commented May 29, 2021

lardieri commented Jul 10, 2021

lardieri commented Aug 2, 2021

MichaelCurrin commented Aug 3, 2021

mattr- commented Aug 3, 2021

Use owner name as site title for User and Organization sites. #197

Use owner name as site title for User and Organization sites. #197

Conversation

lardieri commented Feb 23, 2021 • edited Loading

lardieri commented Feb 23, 2021

mattr- commented Feb 23, 2021

mattr- left a comment

Choose a reason for hiding this comment

MichaelCurrin commented Mar 1, 2021

MichaelCurrin commented Mar 1, 2021 • edited Loading

MichaelCurrin commented Mar 1, 2021

Tests

MichaelCurrin left a comment

Choose a reason for hiding this comment

lardieri commented Mar 1, 2021

Tests

MichaelCurrin commented Mar 1, 2021 • edited Loading

lardieri commented May 29, 2021

lardieri commented Jul 10, 2021

lardieri commented Aug 2, 2021

MichaelCurrin commented Aug 3, 2021

mattr- commented Aug 3, 2021

lardieri commented Feb 23, 2021 •

edited

Loading

MichaelCurrin commented Mar 1, 2021 •

edited

Loading

MichaelCurrin commented Mar 1, 2021 •

edited

Loading