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

Leverage metadata to populate gems urls v2 #1234

Merged
merged 4 commits into from
Jan 6, 2017

Conversation

kbrock
Copy link
Contributor

@kbrock kbrock commented Mar 28, 2016

High Level Goals

  1. The app knows too much about the url generation process. Provide a decorator versioned_links to simplify this logic.
  2. More importantly: All metadata for a gem comes from the gemspec file except for urls. Provide a mechanism to put urls into the gemspec.

goal 1 makes the process of fetching the urls out of gemspec trivial - 2 lines of change. If we are unsure about #2, then can we at least focus/agree on the first goal?

Commentary

This is an alternative to #724. It is in response to comment from @qrush [here] For implementing #1127 - take urls from rubygems.metadata and retire linkset.

@arthurnn I thought it would be simpler to keep this separate from #724. Let me know if you want me to add metadata (and/or add change log) support directly to this PR. I added in the metadata as the second commit

before:

Links on the pages are generated from Links and Version objects. This is needed because some urls have version.number.
This logic finds it's way into rss, views, helpers, and models. Mostly in rubygems.rb, rubygems_helper.rb, and links.rb.

after:

A decorator LinkedVersion is introduced to simplify the relationship between these objects. Since rss uses this as well, the decorator is kept in app/model/.

future:

Once rubygems.metadata is used by most (all?) gems, drop linkset. (many years from now)

Thanks,
Keenan

UPDATED: hope this is more readable

@kbrock
Copy link
Contributor Author

kbrock commented Mar 28, 2016

fixed cops

@kbrock kbrock changed the title Introduce Links - version specific linkset Leverage metadata to populate gems urls v2 Mar 29, 2016
@dwradcliffe dwradcliffe added this to the Metadata milestone May 26, 2016
@homu
Copy link
Contributor

homu commented May 29, 2016

☔ The latest upstream changes (presumably #1294) made this pull request unmergeable. Please resolve the merge conflicts.

@kbrock kbrock force-pushed the versioned_links branch 2 times, most recently from 6ed35c6 to 091e70a Compare May 30, 2016 01:51
@kbrock
Copy link
Contributor Author

kbrock commented May 30, 2016

@arthurnn / @dwradcliffe Any thoughts on this PR?

I separated this into 2 commits.

The first one is consolidating the version specific knowledge while the second one is moving forward on the meta-data route.

If you want me to pull out the first one for now (as it will not actually change any behavior) just let me know.

@kbrock kbrock force-pushed the versioned_links branch 2 times, most recently from b4de6be to 1639cbf Compare May 30, 2016 12:15
@djberg96
Copy link

djberg96 commented Jul 26, 2016

@drbrain @qrush @dwradcliffe @bf4 @arthurnn @mpapis @segiddins Any objections?

@simi
Copy link
Member

simi commented Jul 27, 2016

What about to move Links model under Rubygem namespace aka Rubygem::Links?

@djberg96
Copy link

djberg96 commented Aug 4, 2016

@simi None of the other models do this.

@homu
Copy link
Contributor

homu commented Nov 6, 2016

☔ The latest upstream changes (presumably #1433) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented Nov 12, 2016

☔ The latest upstream changes (presumably #1487) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented Nov 19, 2016

☔ The latest upstream changes (presumably #1506) made this pull request unmergeable. Please resolve the merge conflicts.

@kbrock kbrock force-pushed the versioned_links branch 2 times, most recently from 37059a4 to a1903e3 Compare December 5, 2016 23:36
@kbrock
Copy link
Contributor Author

kbrock commented Dec 5, 2016

updated to fix conflict. (had extracted a commit into the separate PR)
also added tests around indexed.

@kbrock
Copy link
Contributor Author

kbrock commented Dec 22, 2016

@sonalkr132 / @dwradcliffe would you like me to introduce the Links set as a single pr

This PR is 2 commits:

  1. Create a single model that has the data necessary to represent Links (linkset, version.number, and gemset.name).
  2. Allow Links to also read from version.metadata.

I had thought just consolidating the 2 commits into a single PR would make things simpler for all. But after 10 months, I'm not so sure. And the original PR is even older.

Question: Since the first commit does not change any business logic, would you prefer I pull that out?

@homu
Copy link
Contributor

homu commented Jan 6, 2017

⌛ Testing commit 5c37cc9 with merge 22e08a8...

homu added a commit that referenced this pull request Jan 6, 2017
Leverage metadata to populate gems urls v2

## High Level Goals
1. The app knows too much about the url generation process. Provide a decorator `versioned_links` to simplify this logic.
2. More importantly: All metadata for a gem comes from the `gemspec` file except for urls. Provide a mechanism to put urls into the `gemspec`.

goal 1 makes the process of fetching the urls out of gemspec trivial - 2 lines of change. If we are unsure about #2, then can we at least focus/agree on the first goal?
## Commentary

This is an alternative to #724. It is in response to comment from @qrush [[here]](#1127 (comment)) For implementing #1127 - take urls from `rubygems.metadata` and retire `linkset`.

@arthurnn ~~I thought it would be simpler to keep this separate from #724. Let me know if you want me to add metadata (and/or add change log) support directly to this PR.~~ I added in the metadata as the second commit
## before:

Links on the pages are generated from `Links` and `Version` objects. This is needed because some urls have `version.number`.
This logic finds it's way into rss, views, helpers, and models. Mostly in `rubygems.rb`, `rubygems_helper.rb`, and `links.rb`.
## after:

A decorator `LinkedVersion` is introduced to simplify the relationship between these objects. Since rss uses this as well, the decorator is kept in `app/model/`.
## future:

Once `rubygems.metadata` is used by most (all?) gems, drop linkset. (many years from now)

Thanks,
Keenan

**UPDATED:** hope this is more readable
@homu
Copy link
Contributor

homu commented Jan 6, 2017

☀️ Test successful - status

@homu homu merged commit 5c37cc9 into rubygems:master Jan 6, 2017
@kbrock kbrock deleted the versioned_links branch January 6, 2017 23:20
@simi
Copy link
Member

simi commented Jan 8, 2017

🎊

homu added a commit to rubygems/rubygems that referenced this pull request Mar 8, 2017
Validate metadata link keys

# Description:

We intend to deprecate form used to edit linkset of gems. Metadata will be the only way to specify homepage, codebase, documentation, wiki, mailing list and issue tracker urls.

Related: rubygems/rubygems.org#1234
rubygems/rubygems.org#1557
# Tasks:

- [x] Describe the problem / feature
- [ ] Write tests
- [ ] 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).
@sonalkr132
Copy link
Member

@kbrock Can you please explain your reasoning for using metadata specs named *_uri? I understand the need for a long method, however, I think using the shorter version (home, code etc) in metadata would be easier to use.

spec.metadata = {
    "home"      => "https://some.github.io",
    "code"      => "https://github.com/some/gem",
    "docs"      => "https://www.rubydoc.info/gems/some/0.0.1",
  }

vs:

spec.metadata = {
    "homepage_uri"       => "https://some.github.io",
    "source_code_uri"    => "https://github.com/some/gem",
    "documentation_uri"  => "https://www.rubydoc.info/gems/some/0.0.1"
  }

@mpapis
Copy link

mpapis commented Apr 17, 2017

@sonalkr132 the key names must be informative, there is more names https://github.com/rubygems/rubygems.org/pull/1234/files#diff-b77c80170c3a1963bb4915b58c139b43R3 and they could be confusing, long versions do not leave place for misinterpretation

homu added a commit to rubygems/rubygems that referenced this pull request Apr 19, 2017
Fix metadata link key names

# Description:

I had used incorrect names for metadata link keys. Related: rubygems/rubygems.org#1234 (comment)

Good thing it wasn't released with 2.6.11. Is there anything I can do to make sure that my commits are in next release? I need to update guides.
# 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).
@bquorning bquorning mentioned this pull request Sep 12, 2017
11 tasks
bquorning added a commit to bquorning/rubocop that referenced this pull request Sep 12, 2017
Makes it easy to programatically find the source code and changelog for rubocop,
using the rubygems API. Also adds a changelog link etc. to
https://rubygems.org/gems/rubocop

See rubygems/rubygems.org#1234 and
https://github.com/rubygems/rubygems.org/blob/master/app/models/links.rb for
more details.

I did not add `wiki_uri`, `mailing_list_uri` and `download_uri` because 1) the
only wiki I could find (https://github.com/bbatsov/rubocop/wiki) looks
abandoned; 2) there is no mailing list that I know of, and 3) rubygems.org will
add a download link, e.g. https://rubygems.org/downloads/rubocop-0.49.1.gem if
we don't add one.

Also changing `s.homepage` to use https protocol.
bbatsov pushed a commit to rubocop/rubocop that referenced this pull request Sep 12, 2017
Makes it easy to programatically find the source code and changelog for rubocop,
using the rubygems API. Also adds a changelog link etc. to
https://rubygems.org/gems/rubocop

See rubygems/rubygems.org#1234 and
https://github.com/rubygems/rubygems.org/blob/master/app/models/links.rb for
more details.

I did not add `wiki_uri`, `mailing_list_uri` and `download_uri` because 1) the
only wiki I could find (https://github.com/bbatsov/rubocop/wiki) looks
abandoned; 2) there is no mailing list that I know of, and 3) rubygems.org will
add a download link, e.g. https://rubygems.org/downloads/rubocop-0.49.1.gem if
we don't add one.

Also changing `s.homepage` to use https protocol.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants