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

Disambiguation of SECU #5368

Merged
merged 3 commits into from Sep 20, 2021
Merged

Disambiguation of SECU #5368

merged 3 commits into from Sep 20, 2021

Conversation

cicku
Copy link
Contributor

@cicku cicku commented Sep 19, 2021

Previously SECU was only for the credit union based in North Carolina, and it has been wrongly used in OSM when mapping another credit union based in Maryland.

This PR requires review before merging, to avoid further confusion.

@cicku cicku changed the base branch from Delete-name-tag-for-charging-stations to main September 19, 2021 21:59
@tas50
Copy link
Contributor

tas50 commented Sep 19, 2021

You have another commit that snuck in here too

@arch0345
Copy link
Collaborator

You have another commit that snuck in here too

Looks like they committed from a stale branch. This was probably a mistake as these changes were rejected in #4985 in favor of preserveTags.

@cicku
Copy link
Contributor Author

cicku commented Sep 19, 2021

Yes I didn't know why that the PR was trying to merge into an abandoned branch. Just corrected it but it pulled in another one.

Would you please only merge my commit later? I'm currently not able to edit from my phone.

@arch0345 arch0345 marked this pull request as draft September 19, 2021 22:27
@cicku cicku marked this pull request as ready for review September 20, 2021 03:35
@cicku
Copy link
Contributor Author

cicku commented Sep 20, 2021

Do I have to remove the id if changing the existing entry?

@bhousel
Copy link
Member

bhousel commented Sep 20, 2021

Do I have to remove the id if changing the existing entry?

It’s not needed - next time someone runs npm run build It will check/fix all the ids.

@cicku
Copy link
Contributor Author

cicku commented Sep 20, 2021

Can this be merged now?

@arch0345
Copy link
Collaborator

Can this be merged now?

Since these two entries have the same name and locationSet, they will get the same id. I'll disambiguate these locationSets sometime today.

@arch0345
Copy link
Collaborator

Thanks @cicku for your patience!

@cicku
Copy link
Contributor Author

cicku commented Oct 6, 2021

@arch0345 Hi, may I ask how soon will I be able to use the new tag suggestion? Currently I can only see the one in NC as usual.

Is a new iD release required because of duplicate "id"s?

@bhousel
Copy link
Member

bhousel commented Oct 6, 2021

It seems to be working now?

Add node, search for "secu" in Maryland:

Screen Shot 2021-10-06 at 10 13 57 AM

Add node, search for "secu" in North Carolina:

Screen Shot 2021-10-06 at 10 14 27 AM

iD and RapiD pull the latest compatible NSI data from a CDN, and current version is from a release a few days ago:
Screen Shot 2021-10-06 at 10 18 12 AM

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

4 participants