Introduce ISO 3166-2 country codes for creator directories #83

xamanu · 2017-10-24T21:35:47Z

The more creator implementations we are getting, the more it will be needed to have a bit more consistent naming of those. Therefore I would like to introduce a ISO 3166-2 country code for each creator and focus - if possible - on the city name rather than the network or transport agency.

What do you think?

AltNico · 2017-10-25T00:29:13Z

Having both the country and the town in the name of the creators is great in my opinion. Only problem I see is that there could be different companies in one town which could be solved by appending its name to the existing country-town combination.

xamanu · 2017-10-25T06:46:57Z

Yes, I also had in mind your thought, @AltNico. We could do the following:

Currently:

Everybody names the directory of the custom city implementation as they want. Some use the city name, like accra, some the network, like incofer, and some the operator, like fenix.

This Pull Request

Introduces a more standardized way: COUNTRYCODE-CITYNAME

However this is not enough, as @AltNico was describing, and also we have the first edge case here: cr-incofer, while "incofer" is the network, on purpose I didn't rename it to "San Jose" or anything, so to be not mixed up with bus networks.

Idea

I'd be happy to extend this Pull Request to handle multiple hyphens. In this case we can encourage to use: COUNTRYCODE-CITYNAME-NETWORK

This would bring us to:

br-florianopolis-sim (class name: BrFlorianopolisSim)
cr-valle-central-incofer (class name: CrValleCentralIncofer)
ni-managua-irtramma (class name: NiManaguaIrtramma)

The name will become longer but the naming more consistent and it perfectly handles the edge cases. What do you think about this one?

Skippern · 2017-10-25T08:40:22Z

What with Companies serving Multiple cities? My BR-transcol and BR-seletivo serves Vila Velha, Viana, Cariacica, Vitória and Serra with two lines extending into Guarapari and Fundão, I am also looking at intermunicipal routes with Planeta, covering most of southern Espírito Santo. I think country code is highly necessary as city or company name might be repeated in different countries, and also for sorting purposes My suggestion: <ISO country code>[-<city/state/region>][-<company name>]

…

Sent from my iPhone

On 25 Oct 2017, at 09:46, Felix ***@***.***> wrote: Yes, I also had in mind your thought, @AltNico. We could do the following: Currently: Everybody names the directory of the custom city implementation as they want. Some use the city name, like accra, some the network, like incofer, and some the operator, like fenix. This Pull Request Introduces a more standardized way: COUNTRYCODE-CITYNAME However this is not enough, as @AltNico was describing, and also we have the first edge case here: cr-incofer, while "incofer" is the network, on purpose I didn't rename it to "San Jose" or anything, so to be not mixed up with bus networks. Idea I'd be happy to extend this Pull Request to handle multiple hyphens. In this case we can encourage to use: COUNTRYCODE-CITYNAME-NETWORK This would bring us to: br-florianopolis-sim (class name: BrFlorianopolisSim) cr-valle-central-incofer (class name: CrValleCentralIncofer) ni-managua-irtramma (class name: NiManaguaIrtramma) The name will become longer but the naming more consistent and it perfectly handles the edge cases. What do you think about this one? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

grote · 2017-11-05T15:03:35Z

I am not actually sure we need special conventions for creator naming at this point.

xamanu · 2017-11-28T22:55:24Z

A separation by country code would be useful. If not this can quickly become long list of confusing names. It's not a big thing, but a nice improvement. I don't understand why this shouldn't go in...?

grote · 2017-11-29T14:00:54Z

I don't say it can't go in. I just think that it is still not a long list of confusing names. Can't we cross that bridge when we get there?

xamanu · 2017-11-29T14:08:58Z

Sure, this is not urgent now. I'm happy to keep it in the pipeline until there are more creators.

As a constraint: it will be more difficult to change the names of the existing directories, the more creators and maintainers are involved. A good and solid practice can save us time later on.

grote · 2017-11-29T14:18:10Z

Indeed, but do we have a consensus on how to standardize the creator names?

Once we have one, I would like to see a test that checks that all existing creators adhere to that standard, so it gets automatically checked when new creators are added.

Also, we have now creator specific tests in master. These names should be standardized as well then.

xamanu · 2017-11-29T14:29:36Z

Yes, better to come to an agreement before:

I would suggest: <ISO 3166-2 two letter country code>-<city/state/region>-<network/operator name> (requiring all three parts), like ni-managua-irtramma
All with small letters; only ASCII letters and numbers

grote · 2017-11-29T14:50:21Z

@nlehuby @jamescr what do you think about the name standardization @xamanu is proposing above?

I had some reservations about adding the <network/operator name>, because I am planning to merge GTFSs for different operators eventually, but this is probably a post-processing to be done outside the scope of osm2gtfs.

ialokim · 2017-11-29T15:07:35Z

I agree that it is necessary to find a naming convention. Also, I think it's an important possibility to add the <network/operator name> to distinguish between different operators, but what about leaving this part optional, because in some cases there's no official network or one simple operator name.

In Estelí, the urban buses are served by different operators, but are coordinated by the local government. What would you propose as network in this case? ni-esteli-alcaldia? 🤔

I'm also not sure about the <city/state/region> part, because we are thinking of adding a GTFS configuration file for the national bus services.

xamanu · 2017-11-29T20:53:41Z

Yes, I agree, maybe it would be good to leave the network/operator optional: <ISO 3166-2 two letter country code>-<city/state/region>[-<network/operator name>]
If you wanted to unite all of Nicargua into one I would name it ni-national-mit (mit = Ministerio de Transporte)

AltNico · 2017-11-30T02:34:17Z

Yes, I agree, maybe it would be good to leave the network/operator optional: <ISO 3166-2 two letter country code>-<city/state/region>[-<network/operator name>]

👍 for this because I think it is very important to at least the nation code part of the creators name. We should decide this yet, with only few creators, because later it will be harder to do.

nlehuby · 2017-11-30T14:20:10Z

Hi,

<ISO 3166-2 two letter country code>-<city/state/region>[-<network/operator name>] seems good, but may not cover all cases.
For instance, I cannot choose a country code for these cases :

international coaches (if I want to create a Flixbus GTFS from OSM)
cross border lines (for instance in Euskadi)

but I guess eu-Europe-Flixbus and eu-Euskadi-Pesa would be ok.

We should not overthink this as we don't know yet how OSM2GTFS will be used...so I'm ok with this convention.
For Accra, it would be gh-accra, because there is no branded network, and many differents operators.

xamanu · 2017-12-06T09:18:25Z

Seems like we have a consensus on the creator name standardization.

xamanu · 2017-12-06T22:18:14Z

I'm happy to write the requested extension to this PR and the tests, but in order to not go too much in parallel things that are becoming a beast to rebase on each other, I'd prefer to see first those PRs getting in:

Allow schedule source to be specified #91 - Allow schedule source to be specified
WIP: Data structure 2.0 #96 - New data structure and default creators
Add config file for Esteli, Nicaragua network #92 - Add Esteli, based on the newly introduced standard creators
Add config file for Managua, Nicaragua network #88 - Add Managua, based on the newly introduced standard creators

Thanks.

jamescr · 2017-12-20T15:42:40Z

As @grote I'm planning to incorporate bus routes to the generated GTFS. In this case the name for the creator will be cr-gam, GAM stands for Large Metropolitan Area (Gran Area Metropolitana).

xamanu · 2017-12-23T21:49:31Z

Rerolled, now based on PR #99. This now includes also proper tests for naming of files and directories for additional creators.

xamanu · 2018-01-24T21:52:12Z

Rebased on latest upstream master branch (which included #99 and other nice stuff)

xamanu · 2018-02-02T19:27:58Z

This PR is months old and has been rebased several times. I included all remarks (on getting a consensus and including tests for this).

@grote, please let me know when you are considering to merge this. Then I will rebase again. Keeping it up to date without a perspective is a Sisyphus job, especially because this PR consists in some renaming.

grote · 2018-02-26T13:46:50Z

Sorry @xamanu, this branch has conflicts again :/

xamanu · 2018-02-26T13:50:47Z

Yes, it has. As said in the last comment I will only prepare it again, when it has a clear perspective of being merged. Please confirm, and I'm happy to rebase it to the lastest master.

grote · 2018-02-26T13:52:46Z

Maybe then it is better to get some reviews from the others first since it touches all creators?

xamanu · 2018-02-27T00:00:15Z

@nlehuby, @jamescr and @ialokim may I ask you for a review from your side, please. Thanks!

nlehuby · 2018-02-27T14:45:23Z

osm2gtfs/core/creator_factory.py

+                class_name_selector += part.capitalize()
+        else:
+            class_name_selector = selector.capitalize()
+


can we avoid to duplicate the class_name_selector creation code in each function ?

+1 That would be much nicer indeed!

nlehuby · 2018-02-27T14:48:16Z

osm2gtfs/tests/README.md

 #### Description

-There are 3 unittests for the Accra GTFS generation
+There are 3 unit tests for the Accra GTFS generation


actually, this is not an unit test, but an acceptance one.

nlehuby · 2018-02-27T14:49:27Z

osm2gtfs/tests/README.md

-The generated GTFS is checked against a reference GTFS file (accra_tests.zip.ref)
-in the `tests/fixtures/accra/` folder. For the moment, only the size of each GTFS file is compared to the reference.
+The generated GTFS is checked against a reference GTFS file (gh_accra_tests.zip.ref)
+in the `tests/creators/fixtures_gh_accra/` folder. For the moment, only the size of each GTFS file is compared to the reference.


should be tests/creators/fixtures/gh_accra/ and not tests/creators/fixtures_gh_accra/

ialokim

Thanks @xamanu, good work!

Please see my inline comments!

ialokim · 2018-02-27T17:34:32Z

osm2gtfs/core/creator_factory.py

+                class_name_selector += part.capitalize()
+        else:
+            class_name_selector = selector.capitalize()
+


+1 That would be much nicer indeed!

ialokim · 2018-02-27T17:44:55Z

osm2gtfs/creators/ni_esteli/ni_esteli.json

-    "output_file": "data/ni-esteli.zip",
-    "selector": "esteli"
+    "output_file": "data/ni-esteli-gtfs.zip",
+    "selector": "ni_esteli"


Why did you go for _ in the selector's case? I think it would be easier and more consistent to use - here (as in the output file, for example) too.

The selector is meant to correspond to the file and directory name of the creator. In Python module names underscores are allowed, according to PEP8, but hyphens aren't. So, to me it seems much more consistent to use the underscore, according to the creators files and directory. Anyway the output file can be specified as wished, there is no rule here.

OK, I understand, thanks for your explanation!

ialokim · 2018-02-27T17:51:45Z

osm2gtfs/tests/core/tests_creator_factory.py

+                    ' " doesn\'t fit the naming convention.')
+
+                # Check for existing configuration file
+                config_file_path = path + selector + "/" + selector + ".json"


Why has it to be ni_esteli/ni_esteli.json for the config file? I think it would be a lot more simple to change this naming convention to only config.json in all the creators subfolders, so you don't have to type the (potentially long) selector name twice while running osm2gtfs.

I see this would be another change added to this PR, but as you're already renaming the json files, I think it would be the right place.

Yes, I agree with you. It would be nicer, but I wanted to keep this PR smallest possible. However as you are also suggesting it, I will apply it to the next rebase here.

Thanks a lot!

ialokim · 2018-02-27T17:52:29Z

osm2gtfs/tests/core/tests_creator_factory.py

+                        path + selector):
+
+                    for filename in inner_files:
+                        # Check the pyhton files (and ignore the others)


ialokim · 2018-02-27T17:54:26Z

osm2gtfs/tests/core/tests_creator_factory.py

+                                "This file is not well named: " + filename)
+
+                            # Check for valid creators
+                            split_filename = filename.split("_")


This won't work for feed_info (in creators list above).

Thanks for noticing! I fixed it.

Looks a bit ugly, but I'm okay with it.

The alternative is to rename feed_info (as in transitfeed) to feedinfo, to avoid the different meaning of the underscore. However I suggest to discuss and apply this in a separate PR.

That's okay for me! 👍

ialokim · 2018-02-27T17:56:18Z

osm2gtfs/tests/core/tests_creator_factory.py

+                check = False
+        else:
+            return False
+        return check


Generally, +1 for adding this test!

Correct me if I'm wrong but this only tests the filenames and not the class names as required by the CreatorsFactory. Not sure how difficult it would be to implement this tests too.

Added the desired test.

xamanu · 2018-02-27T21:17:35Z

Thank you very much @nlehuby and @ialokim for your reviews. I will incorporate your suggestions soon.

xamanu · 2018-02-28T02:56:12Z

Here is a new version of this PR for your consideration. It includes the improvements based on the feedback by the reviews of @nlehuby and @ialokim, it is rebased on the latest upstream master and test checks passed successfully.

By the way, this PR is introducing the first tests for parts of osm2gtfs' core \o/

ialokim

Looks great. I've just one question.

ialokim · 2018-02-28T17:23:08Z

osm2gtfs/tests/core/tests_creator_factory.py

+
+                            # Snake to camel case to test for correct class names
+                            classname = ''.join(x.capitalize() or
+                                                '_' for x in filename[:-3].split('_'))


Not sure if this is because of my lack of understanding, but why exactly did you use this or '_' part?

I took it from this source, but it works also with out the or '_' part. So, I took it out.

Please don't copy code whose copyright isn't yours into the project.

Thanks for the reminder! You are very right!! I was wrongly assuming a public domain permission, which is not the case (only Creative Commons licensed).

Anyway, with the latest changes it is not coming from any particular source, but rather a common knowledge based on a web research and my own knowledge, the used code passage as it is - adapted to our code - can be found in variations in numerous public places and doesn't belong to one creator, but explicitly here myself.

Introduce tests for creator factory

xamanu · 2018-02-28T19:59:35Z

Thank you for your review @grote and @ialokim. This PR seems to be quite ready to be merged in.

@jamescr: This PR renames the creator incofer to cr_gam, as you mentioned here earlier. Can you please confirm, that you are fine with this renaming?
@nlehuby: Can you have a look again and confirm the PR after applying the suggestions?

Thanks to everybody! 😄

ialokim

Perfect, I'm absolutely fine with this now!

nlehuby

👍

xamanu mentioned this pull request Nov 29, 2017

Add config file for Managua, Nicaragua network #88

Closed

xamanu added the help wanted label Dec 11, 2017

xamanu added enhancement and removed help wanted labels Dec 23, 2017

xamanu mentioned this pull request Dec 24, 2017

Simplify region configurations #107

Open

xamanu requested a review from grote January 5, 2018 19:14

xamanu added the ready label Jan 6, 2018

xamanu removed the ready label Feb 2, 2018

xamanu mentioned this pull request Feb 25, 2018

Add support for national buses and ferries of Nicaragua #130

Open

nlehuby requested changes Feb 27, 2018

View reviewed changes

xamanu requested a review from jamescr February 27, 2018 15:34

ialokim suggested changes Feb 27, 2018

View reviewed changes

xamanu requested a review from nlehuby February 28, 2018 03:00

xamanu added the ready label Feb 28, 2018

This was referenced Feb 28, 2018

Regression tests #75

Closed

Acceptance tests for Managua, Nicaragua creator #131

Merged

ialokim reviewed Feb 28, 2018

View reviewed changes

grote approved these changes Feb 28, 2018

View reviewed changes

Introduce ISO 3166-2 country codes for creator directories

692e30d

Introduce tests for creator factory

ialokim approved these changes Feb 28, 2018

View reviewed changes

xamanu mentioned this pull request Feb 28, 2018

Core tests #116

Open

nlehuby approved these changes Mar 1, 2018

View reviewed changes

grote merged commit b4b8446 into grote:master Mar 1, 2018

xamanu deleted the creator-hyphens branch April 3, 2018 23:04

Introduce ISO 3166-2 country codes for creator directories #83

Introduce ISO 3166-2 country codes for creator directories #83

Conversation

xamanu commented Oct 24, 2017

AltNico commented Oct 25, 2017 via email

xamanu commented Oct 25, 2017

Skippern commented Oct 25, 2017 via email

grote commented Nov 5, 2017

xamanu commented Nov 28, 2017

grote commented Nov 29, 2017

xamanu commented Nov 29, 2017

grote commented Nov 29, 2017

xamanu commented Nov 29, 2017 • edited Loading

grote commented Nov 29, 2017

ialokim commented Nov 29, 2017

xamanu commented Nov 29, 2017

AltNico commented Nov 30, 2017

nlehuby commented Nov 30, 2017

xamanu commented Dec 6, 2017

xamanu commented Dec 6, 2017 • edited Loading

jamescr commented Dec 20, 2017

xamanu commented Dec 23, 2017

xamanu commented Jan 24, 2018

xamanu commented Feb 2, 2018 • edited Loading

grote commented Feb 26, 2018

xamanu commented Feb 26, 2018

grote commented Feb 26, 2018

xamanu commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialokim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xamanu Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xamanu Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xamanu commented Feb 27, 2018

xamanu commented Feb 28, 2018 • edited Loading

ialokim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xamanu Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

xamanu commented Feb 28, 2018 • edited Loading

ialokim left a comment

Choose a reason for hiding this comment

nlehuby left a comment

Choose a reason for hiding this comment

xamanu commented Nov 29, 2017 •

edited

Loading

xamanu commented Dec 6, 2017 •

edited

Loading

xamanu commented Feb 2, 2018 •

edited

Loading

xamanu Feb 27, 2018 •

edited

Loading

xamanu Feb 28, 2018 •

edited

Loading

xamanu commented Feb 28, 2018 •

edited

Loading

xamanu Feb 28, 2018 •

edited

Loading

xamanu commented Feb 28, 2018 •

edited

Loading