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

Added all missing regional lines from Baden-Württemberg #79

Closed

Conversation

oneiricbotcelot
Copy link
Contributor

I added all the missing regional lines and unified existing ones according to the official map from bwegt as stated in this comment.

Simultaneously I fixed some ordering and spacing issues.

@vainamov
Copy link
Member

I would like to veto this PR. Apart from the fact, that I think it does way more than it's supposed to, generalizing all line names to have a space between the letters and numbers is simply wrong.

I would assume, that all contributions prior to this have used the correct spacing as proven by their included sources. Without a doubt (because I added them myself) at least the night bus lines from regioBus were wrongly changed.

@oneiricbotcelot
Copy link
Contributor Author

oneiricbotcelot commented Dec 22, 2023

Let me further elaborate on what I did in this pull request:

  • As stated I added nearly all lines from the official map from bwegt.
  • For some existing lines in Baden-Württemberg (i.e. the lines operated by SWEG) I changed the colors to correspond to the just mentioned map.
  • I removed the prefix STR in the lineName for the trams in Ulm and Freiburg to unify the data corpus for trams. While I initially submitted the colors of both trams back in October, I purposely added STR based on the way Träwelling displayed these lines before line colors were introduced.
  • Some entries in the sources.json were not strictly ordered by shortOperatorName. I fixed that.
  • In a few cases entries in lines-colors.csv were not strictly sorted by shortOperatorName and lineName, in example the regional lines in NRW were ordered by ascending numbers instead of RB before RE. I fixed that, too.
  • I changed the shortOperatorName for the Westfrankenbahn from db-regionetz-westfrankenbahn to db-wfb to introduce the commonly used abbreviation.
  • To my understanding lineNames consist of two things most of the time: the category and the line's number, which ironically can involve letters as well. In an attempt to further standardize the dataset I separated the categories Regionalbahn (RB), Regionalexpress (RE), Interregio-Express (IRE), Metropolexpress (MEX), S-Bahn (S) and U-Bahn (U) and the line's number with a blank across the whole CSV file.
  • I also did the separation for some night buses (i.e. N57 --> N 57 and NL2 --> NL 2) - maybe @vainamov is right and I did overshoot here.

In my opinion it is a good thing to separate category and line number:

  1. It enhances readability.
  2. Many sources do not have a definitive way of calling these lines and are not consistent. For example bwegt does not use separation blanks in the map itself, but does so in articles or other documents on its website. I just checked, Go-Ahead (in Baden-Württemberg and Bayern) behaves the same.
  3. In the electronic railway guide they also seem to use separation blanks for most regional lines. I attached the following screenshot for KBS 670.
    screenshot for KBS 670.

I am happy to discuss these changes and can make changes if deemed neccessary.

@vainamov
Copy link
Member

vainamov commented Dec 22, 2023

@oneiricbotcelot I thank you for your submission over all. New entries and fixes regarding the order of some items are always a great addition! As I said, I myself would have preferred, if some of the changes would have been made in seperate PRs. It would allow to quickly approve the straight-forward changes and it makes reviewing the diffs easier, especially when things get reordered and added at the same time.

For some existing lines in Baden-Württemberg (i.e. the lines operated by SWEG) I changed the colors to correspond to the just mentioned map.

I'm not from Baden-Württemberg so I don't feel like I can make a decision on this, but the readme states "If a single line operates in multiple transport networks, the color communicated by the operator shall be preferred." @jheubuch?

I removed the prefix STR in the lineName for the trams in Ulm and Freiburg to unify the data corpus for trams. While I initially submitted the colors of both trams back in October, I purposely added STR based on the way Träwelling displayed these lines before line colors were introduced.

Again, welcome and (looking at the sources) valid change.

  1. It enhances readability.
  2. Many sources do not have a definitive way of calling these lines and are not consistent. For example bwegt does not use separation blanks in the map itself, but does so in articles or other documents on its website. I just checked, Go-Ahead (in Baden-Württemberg and Bayern) behaves the same.

I agree with you, inconsistencies across the refrence maps and plans are a thing and a pain to work with. I also value your attempt to increase readability, because I agree it's something that should be done, yet I think it's not something that we should do.

As I see it, the idea of this repository is just to collect data about the different line styles. Some of the colors might be worth changing because of low contrast (see some quick examples I found below), yet we store the original values without adjustments. I don't see why we should deviate from this strategy for line names.

grafik
grafik
grafik

The consumers of this dataset should be able decide for themselves, whether they want to display (or use in whatever other way) this data accurate as is or whether they feel the need to make it more accessible.

@marhei
Copy link
Contributor

marhei commented Dec 22, 2023

I think @vainamov is right: You should split your PRs to increase the possibility of discussion and minimise the risk of conflict!

Anyway: Thanks for your work!

@oneiricbotcelot
Copy link
Contributor Author

Thanks for your feedback. I will start pushing the changes made in this pull request in small packages in the next days.

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.

3 participants