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

Add ref and railway:track_ref fields to railway=rail and =switch presets #1083

Merged
merged 12 commits into from Jan 31, 2024
Merged

Conversation

gy-mate
Copy link
Contributor

@gy-mate gy-mate commented Jan 19, 2024

  • railway=rail
  • railway=switch
    • Create ref=* (Switch Number) field
    • Add this field to its preset

data/presets/railway/rail.json Outdated Show resolved Hide resolved
data/presets/railway/switch.json Show resolved Hide resolved
@gy-mate gy-mate requested a review from tordans January 19, 2024 09:54
Copy link
Collaborator

@tordans tordans left a comment

Choose a reason for hiding this comment

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

LGTM But disclaimer: I know litte about railway-mappging :-).

Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Apart from a few minor syntax errors (see below), this looks fine for me as well.

Only one question: the ref field seems to be quite a bit more common on railway=rail objects (see taginfo). What was your reasoning behind putting the ref field into moreFields? Are the track numbers usually easier to map?

data/presets/railway/rail.json Outdated Show resolved Hide resolved
data/presets/railway/rail.json Outdated Show resolved Hide resolved
@tyrasd
Copy link
Member

tyrasd commented Jan 24, 2024

One more suggestion: As switches only have a single reference number, it would be sufficient to use the generic field for the ref tag, wouldn't it? This would have the benefit of being slightly less effort to translate and maintain. Or is Switch Number a technical term?

Applied suggestions from code review

Co-authored-by: Martin Raifer <martin@raifer.tech>
@gy-mate
Copy link
Contributor Author

gy-mate commented Jan 24, 2024

What was your reasoning behind putting the ref field into moreFields? Are the track numbers usually easier to map?

@tyrasd Yeah, @tordans suggested this and I agree with him:

railway:track_ref would be commonly known but ref (ref_rail) is something that only external data or professionals would know

@tyrasd
Copy link
Member

tyrasd commented Jan 24, 2024

Ok, that makes sense now. 👍

Copy link

🍱 You can preview the tagging presets of this pull request here.

@tyrasd tyrasd added this to the v6.7 milestone Jan 24, 2024
@gy-mate
Copy link
Contributor Author

gy-mate commented Jan 24, 2024

As switches only have a single reference number, it would be sufficient to use the generic field for the ref tag, wouldn't it? This would have the benefit of being slightly less effort to translate and maintain. Or is Switch Number a technical term?

@tyrasd Well it's not really an essential technical term. You're right, there is no need to overcomplicate, it's still obvious without ref_switch. Reverted those commits.

@gy-mate gy-mate requested a review from tyrasd January 24, 2024 15:43
@tyrasd tyrasd merged commit 96a4600 into openstreetmap:main Jan 31, 2024
5 checks passed
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

3 participants