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

Update browse_helper.rb #814

Closed
wants to merge 3 commits into from
Closed

Update browse_helper.rb #814

wants to merge 3 commits into from

Conversation

pyrog
Copy link

@pyrog pyrog commented Oct 15, 2014

Manage name:wikipedia, subject:wikipedia… name:wikidata…

Manage name:wikipedia, subject:wikipedia… name:wikidata…
@@ -117,7 +117,8 @@ def wikipedia_link(key, value)
# Some k/v's are wikipedia=http://en.wikipedia.org/wiki/Full%20URL
return nil if value =~ /^https?:\/\//

if key == "wikipedia"
# match wikipedia or xxxxx:wikipedia
if key =~ /^(?:.*:)*wikipedia$/
Copy link
Contributor

Choose a reason for hiding this comment

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

Here (and on line 158 as well), would it be more efficient to use the String end_with() method? Not tested

@HolgerJeromin
Copy link
Contributor

Have you looked at the discussion at #788 ?

@pyrog
Copy link
Author

pyrog commented Oct 15, 2014

I decided against a catch-all to discourage misspelled keys. If other prefixes will get popular, one could add them easily in the future.
Ok, i didn't find your message before 😉

So tags with the following keys are the only ones that get linked:

  • architect:wikidata
  • artist:wikidata
  • brand:wikidata
  • operator:wikidata
  • subject:wikidata

Not yet implemented ?

So i propose this expression :

if key =~ /^(?:(architect|artist|brand|name|operator|subject):)?wikipedia$/

@tomhughes
Copy link
Member

Didn't we decide against all these extra tags on a previous pull request?

@tomhughes
Copy link
Member

OK so it was only the etymology thing that was vetoed at that point. But I see now it's expanded to adding lots of wikipedia tags as well! Is there some sort of sane end point to how much metadata people want to try and maintain...

@pyrog
Copy link
Author

pyrog commented Oct 15, 2014

Sorry english is not my mother language 😉

I understood that the problem was to have multi values for wikidata keys, not extra prefix keys

@tomhughes
Copy link
Member

I only read the old ticket quickly but I thought the main problem was that we didn't think the etymology tags belonged in OSM in the first place and as a result we thought they shouldn't be nicely rendered as that would just encourage people to add them.

@pyrog
Copy link
Author

pyrog commented Oct 15, 2014

see this regexp to match all cases : http://regexr.com/39o1g

^(?:(architect|artist|brand|name|operator|subject):)?wikipedia(?:(?::)(\S*))?$

and

^(?:(architect|artist|brand|name|operator|subject):)?wikidata$

@floscher
Copy link
Contributor

@pyrog: The problem isn't that it is not implemented, but that there is no consensus on the question, if those wiki(data|pedia)-tags with prefixes should get linked.

In #788 I've already implemented this for wikidata-tags.

Please keep in mind, that the tags with prefixes can have multivalues, the simple tags without prefix only have one value.

Also only a limited set of tags should be accepted, so things like opperator:wikidata or architekt:wikipedia won't get linked.

@pyrog
Copy link
Author

pyrog commented Oct 18, 2014

@pyrog: The problem isn't that it is not implemented, but that there is no consensus on the question, if those wiki(data|pedia)-tags with prefixes should get linked.

In #788 I've already implemented this for wikidata-tags.

Ok, but I can't see the link on architect:wikidata for this object : http://www.openstreetmap.org/way/219200232

Also only a limited set of tags should be accepted, so things like opperator:wikidata or architekt:wikipedia won't get linked.

I'am agree with you 😄
But name:wiki* is missing (see: Key:wikipedia # Secondary Wikipedia links)

@floscher
Copy link
Contributor

Ok, but I can't see the link on architect:wikidata for this object : http://www.openstreetmap.org/way/219200232

@pyrog: That's because the pull request #788 is still open. That means that I have proposed this change (just like you with this pull-request), but it hasn't been incorporated in the main OSM website.

name:wiki* is missing (see: Key:wikipedia # Secondary Wikipedia links)

As I already mentioned, my pull request only covers the wikidata-tags, not the wikipedia-tags. So I just incorporated the tags proposed in Proposed features/Wikidata. And name:wikidata has neither been proposed, nor is it in use. Instead, the wikidata-version of name:wikipedia is name:etymology:wikidata.
But this tag has been excluded from #788, because it wasn't considered reasonable to promote this tag by linking to WikiData.org.

If needed, any other wikidata-tag could easily be added later by modifying the RegEx in line 167 of browse_helper.rb accordingly.

pyrog added 2 commits January 12, 2015 04:11
Manage name:wikipedia, subject:wikipedia… name:wikidata…
Rebase on current openstreetmap/openstreetmap-website (821a22c)
@pnorman
Copy link
Contributor

pnorman commented Aug 9, 2016

Is #788 a better PR to proceed with?

@floscher
Copy link
Contributor

floscher commented Aug 9, 2016

@pnorman Obviously I am biased towards #788.

But that PR is accompanied by tests and only links a fixed set of wikidata-tags (those mentioned in the wikidata proposal, excluding the etymology-tag) instead of linking all of *:wikipedia=* and *:wikidata=[Qq][1-9][0-9].

The only problem, I currently see is, that browse_helper.rb gets too long in the other PR (138/131 lines), that's why the Travis build currently fails.

@tomhughes
Copy link
Member

I've merged #788 as that had the better implementation.

@tomhughes tomhughes closed this Aug 16, 2016
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.

None yet

6 participants