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

Fix wrong variable used to access tags #9483

Merged
merged 1 commit into from
May 10, 2023

Conversation

biswajit-k
Copy link
Contributor

Fixes #9436. In the code, key is realKey with any prefix removed, like in the given issue realKey="razed:waterway" and key="waterway".
So, to access the value field of tag, realKey should be used and not one with removed prefix, i.e key.

@biswajit-k
Copy link
Contributor Author

biswajit-k commented Feb 3, 2023

The above changes fix the bug as below-
s1

However, I found that if we change the tag to razed:waterway="rapids" the issue panel pops up again which should not be the case as rapids is allowed to take the value of way , as shown below-
s2

Why did this happen? I went deeper and found that the matchTags() function here accessed keys without removing the osmLifecyclePrefix at valueIndex = keyIndex[k].
This issue lies with all the tags containing osmLifecyclePrefixes like proposed:, construction:, razed: etc.

How? This is because the tags are defined only for root keys like { waterway: { ... }, ... } and don't include keys with prefixes like { razed:waterway: { ... }, ... } and are accessed without removing their osmLifecyclePrefixes, like here and here.

I thought about it and could come up with a few solutions as listed below-

  1. Whenever accessing the tag objects like osmAreaKeys, osmAreaKeysExceptions which are defined here, we need to make sure to access the keys with their osmLifecyclePrefixes removed as done here(although the wrong variable is accessed, which is corrected in the above PR).
  2. We can initialize all these tag objects as instances of a factory function say osmTags which has methods to access tags after removing their osmLifecyclePrefixes as below-

function osmTag(tags) {

  var _tags = {};
  
  if(arguments.length) {
  	_tags = tags;
  }
  
  return {

    get: function(key) {
    	return _tags[osmRemoveLifecyclePrefix(key)];
    },
    set: function(tags) {
    	_tags = tags;
    },
    has: function(key) {
    	return osmRemoveLifecyclePrefix(key) in _tags;
    }

  }

}

Then objects can be accessed like osmAreaKeys.has(k) and osmAreaKeys.get(k).

  1. We can define custom get and has methods of objects so that whenever the keys are accessed we internally remove the prefixes and return the values, it would work without changing the way we access objects like osmAreaKeys[k] and k in osmAreaKeys. One way I found to do this is by using the Proxy object(I don't know if this is feasible, but just got an idea so shared).

I believe that the above solutions could help in solving this and prevent bugs from popping up in future. @tyrasd I understand that you have been putting in long hours at work, and I truly appreciate your dedication. If it's not too much of an inconvenience, would you be able to spare some time to provide suggestions? I would greatly value your input and insights. Thank you in advance for your time and consideration.

Please don't mind if I overlooked or said something incorrectly. I'm still learning😄!

@k-yle
Copy link
Collaborator

k-yle commented Apr 19, 2023

For reference, this PR closes #9436, #9491, #9447, openstreetmap/id-tagging-schema#861 and openstreetmap/id-tagging-schema#920

@tyrasd tyrasd added the bug A bug - let's fix this! label Apr 25, 2023
@tyrasd tyrasd merged commit 25cf436 into openstreetmap:develop May 10, 2023
@Higuys153
Copy link

If its been merged when will it be up on the website?

@k-yle
Copy link
Collaborator

k-yle commented Jun 29, 2023

is someone who has permission (@tyrasd @nickrsan?) able to close the issues listed in this comment: #9483 (comment) and ideally link those issues to this PR too?

@nickrsan
Copy link
Collaborator

nickrsan commented Jun 29, 2023

It looks like @1ec5 took care of the remaining open ones on this repository - I have triage permissions here, but not on id-tagging-schema, so someone else will need to take care of openstreetmap/id-tagging-schema#920 which looks to be the only one remaining open.

Thanks for flagging these as ready to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug - let's fix this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

razed:waterway=stream should not be expected as an area
5 participants