Skip to content

Add change log links to gem page#1553

Merged
sonalkr132 merged 3 commits into
rubygems:masterfrom
kbrock:add_changelog_link
Apr 3, 2017
Merged

Add change log links to gem page#1553
sonalkr132 merged 3 commits into
rubygems:masterfrom
kbrock:add_changelog_link

Conversation

@kbrock
Copy link
Copy Markdown
Contributor

@kbrock kbrock commented Jan 26, 2017

Alternative to #728

This reads the changelog uri from the metadata without requiring changing the Linkset table.

The core of the change is in Links:58

It dynamically generates methods for each of the link attributes.
The value is read from the metadata. If it is not present, it falls back to the value in the Linkset.
For changelog (and future uri values), it is not defined in linkset, so a try is used to be flexible here.

/cc @olivierlacan sorry it took me so long.

@kbrock kbrock force-pushed the add_changelog_link branch 2 times, most recently from 54e96bf to 3aab172 Compare January 26, 2017 15:14
@olivierlacan
Copy link
Copy Markdown
Contributor

@kbrock I knew there was something weird about that old migration!

@olivierlacan
Copy link
Copy Markdown
Contributor

I think the tests I had on #728 were pretty thorough, want me to add them back?

@kbrock kbrock changed the title [WIP] Add change log links to gem page Add change log links to gem page Jan 26, 2017
@kbrock
Copy link
Copy Markdown
Contributor Author

kbrock commented Jan 26, 2017

@olivierlacan refresh - linkset does not define changelog. That is the only test I saw. I added them more of an intergration test on the json / xml api.

need tests around view?
you can cherry-pick / steal this stuff back. I rebased on master

@kbrock
Copy link
Copy Markdown
Contributor Author

kbrock commented Jan 26, 2017

@olivierlacan whoa. ok, we'll push forward here.

  1. the i18n failed test are curious. I thought it was defined. maybe I'm misspelling something
  2. the rubocop is easy to fix (was hoping to avoid because this PR looks super small/simple w/o it)
  3. the brakeman issues look like a whole separate PR. sigh

Also, do you have a gem w/ changelog metadata you tested?
I'll put one together and figure out how to load into my db locally. Sorry I haven't introduced in the wild yet :( (I even have a gem w/ no changelog :( )

@dwradcliffe
Copy link
Copy Markdown
Member

@kbrock you'll need to add the locale changes to every language. If you aren't able to translate them (that's totally fine!) you can just add the key with no value.

@kbrock
Copy link
Copy Markdown
Contributor Author

kbrock commented Feb 3, 2017

@dwradcliffe thanks so much

Comment thread app/models/links.rb Outdated

# define getters for each of the uris (both short `home` or long `homepage_uri` versions)
# don't define for download_uri since it has special logic and is already defined
# using a try becaue linkset does not define all the uri attributes
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo becaue => because

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thnx. fixed

@kbrock kbrock force-pushed the add_changelog_link branch 2 times, most recently from b21c36d to efec876 Compare February 7, 2017 16:04
@olivierlacan
Copy link
Copy Markdown
Contributor

@kbrock Other than translations, for which I offer http://keepachangelog.com/ as a guide, do we have any other blockers?

Comment thread config/locales/de.yml Outdated
attributes:
linkset:
bugs: Bug Tracker URL
changelog:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on http://keepachangelog.com/de/0.3.0/ you can use Changelog here too.

Comment thread config/locales/en.yml Outdated
links:
badge: Badge
bugs: Bug Tracker
changelog: Change Log
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd suggest sticking to one-word Changelog. We've had many discussions over on http://keepachangelog.com/ about this and it seems like the most correct in-domain (software) spelling.

Comment thread config/locales/es.yml Outdated
attributes:
linkset:
bugs: URL del registro de Bugs
changelog:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here, Changelog should be fine. http://keepachangelog.com/es-ES/0.3.0/

@indirect
Copy link
Copy Markdown

indirect commented Mar 8, 2017

@kbrock this looks good to me as soon as the last comments from @olivierlacan are addressed. 👍

Comment thread app/models/links.rb Outdated
'wiki' => 'wiki_uri',
'mail' => 'mailing_list_uri',
'bugs' => 'bug_tracker_uri',
'download' => 'download_uri'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we please keep => aligned?

olivierlacan and others added 2 commits March 8, 2017 12:41
This adds a new changelog column to the displays
that change log alongside the existing linkset links.

I originally wrote this with @jaymcgavren but had to recommit because
of really bad rebase clusterpoop. I hope you forgive me, Jay.
This takes changelog out of the Links table

Tried to keep as much of the commits from @olivierlacan and @jaymcgavren
as possible.
@kbrock kbrock force-pushed the add_changelog_link branch from efec876 to 4210796 Compare March 8, 2017 17:56
@kbrock
Copy link
Copy Markdown
Contributor Author

kbrock commented Mar 8, 2017

Thanks @olivierlacan - never occurred to me to look at the source of all of this :(
I went through a few languages and didn't find any special names for changelog.
Let me know if I missed something

@kbrock kbrock force-pushed the add_changelog_link branch from 4210796 to 1aa755c Compare March 8, 2017 18:02
@kbrock
Copy link
Copy Markdown
Contributor Author

kbrock commented Mar 9, 2017

Thanks @sonalkr132 I didn't see that warning when running rubocop locally.

Let me know if you have any other similar comments.

@olivierlacan
Copy link
Copy Markdown
Contributor

@sonalkr132 @kbrock Are we good to go? Or do we want to find more translations for Changelog before we merge this?

@sonalkr132
Copy link
Copy Markdown
Member

LGTM 👍 We will merge as soon as we can.

@sonalkr132 sonalkr132 merged commit 184a774 into rubygems:master Apr 3, 2017
@olivierlacan
Copy link
Copy Markdown
Contributor

@kbrock Curious if you think we should update http://guides.rubygems.org/specification-reference/#metadata with a canonical list of metadata that rubygems.org currently accepts or just a straight up link to Link for now.

sonalkr132 added a commit to sonalkr132/rubygems that referenced this pull request Apr 3, 2017
@sonalkr132
Copy link
Copy Markdown
Member

@olivierlacan Check: ruby/rubygems#1885. guide will be update when we regenerate the doc.

@kbrock kbrock deleted the add_changelog_link branch April 3, 2017 20:09
@olivierlacan
Copy link
Copy Markdown
Contributor

@sonalkr132 Perfect, thank you!

homu added a commit to ruby/rubygems that referenced this pull request Apr 3, 2017
Add changelog to metadata validation

Ref: #1834
rubygems/rubygems.org#1553

# Description:
We need to validate changelog link during `gem build`.

# Tasks:

- [x] Describe the problem / feature
- [ ] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants