-
-
Notifications
You must be signed in to change notification settings - Fork 976
Always show source code on sidebar, even if duplicate link #6118
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
base: master
Are you sure you want to change the base?
Always show source code on sidebar, even if duplicate link #6118
Conversation
72a99f6 to
1f163fd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6118 +/- ##
==========================================
- Coverage 97.24% 94.46% -2.78%
==========================================
Files 476 476
Lines 9785 9851 +66
==========================================
- Hits 9515 9306 -209
- Misses 270 545 +275 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jenshenny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense to me
| end | ||
|
|
||
| should "always include source code even when duplicate" do | ||
| metadata = { "homepage_uri" => "https://example.com", "source_code_uri" => "https://example.com" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| metadata = { "homepage_uri" => "https://example.com", "source_code_uri" => "https://example.com" } | |
| metadata = { "homepage_uri" => "https://example.code", "source_code_uri" => "https://example.code" } |
I think you mean this?
| seen_urls = {} | ||
| links.select do |short, long| | ||
| url = send(long) | ||
| # always include 'code' (source_code_uri) even if URL is duplicate | ||
| if short == "code" | ||
| true | ||
| elsif seen_urls[url] | ||
| false | ||
| else | ||
| seen_urls[url] = true | ||
| true | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly confusing to me since there's a lot of booleans and conditionals, how about this?
Have REQUIRED_INDEXED_LINK_KEYS
| seen_urls = {} | |
| links.select do |short, long| | |
| url = send(long) | |
| # always include 'code' (source_code_uri) even if URL is duplicate | |
| if short == "code" | |
| true | |
| elsif seen_urls[url] | |
| false | |
| else | |
| seen_urls[url] = true | |
| true | |
| end | |
| unique_links = links.uniq do |_short, long| | |
| send(long) | |
| end.to_h | |
| # always include certain URLs even if URL is a duplicate | |
| links.select do |short, _long| | |
| unique_links.key?(short) || REQUIRED_INDEXED_LINK_KEYS.include?(short) | |
| end |
Motivation / Background
Browsing rubygems.org recently I found it strange that some gems show a "Source Code" link in the sidebar while others do not, even if it is set in the gemspec's
source_code_urifield.For example:
Rake does not show "Source Code"
ActiveSupport does show "Source Code"
Both gems specify a
source_code_urivalue in their respective gemspec files.I dove into this and it looks like we are deduping links to declutter the sidebar.
I think this is generally a good idea but would like to propose we always show "Source Code" if it is set. Yes, users can always find the Github page for a gem by clicking the Github Star icon but it feels more intuitive to click "Source Code" if you're looking for the source code.
@etherbob Do you have a strong opinion about this? I don't want to completely revert your change but I also see that even you noted in your original PR, "There's probably also a question to be asked about gem author intent with the duplicate links and whether or not someone might get confused if they don't see a specific link to "source code" for instance if it's been superseded by the "home page" link." I think this is exactly what I'm experiencing and maybe others are as well 😅
Detail
This Pull Request modifies the
unique_linksmethod implementation to remove duplicate links but always includesource_code_uri.I decided to change the
unique_linksmethod to preserve most of the existing behavior. Given that I'm introducing a uniqueness exception tosource_code_uri, should we rename this method? It is still removing duplicate links so maybe it's fine but value others' opinions.