search

MobilityData / gbfs-json-schema

JSON Schema for GBFS feeds, managed by MobilityData. The versions are organized by folders. CC BY 4.0
Apache License 2.0
23 stars 25 forks source link

Different files' classes are no longer namespaced by file #116

Closed testower closed 4 months ago

testower commented 4 months ago

In order to avoid naming conflicts in generated classes, each feed gets its own package [...]

This seems to no longer be true in the mobilitydata published package. Was this changed on purpose?

Alessandro100 commented 4 months ago

Hello, I went through the generated files and noticed that jsonschema2pojo already avoids conflicting names through nomenclature such as GBFSData__1.java. I figured it would be cleaner to import

import org.mobilitydata.gbfs.v3_0.GBFSVehicleStatus

vs

import org.mobilitydata.gbfs.v3_0.vehicle_status.GBFSVehicleStatus
testower commented 4 months ago

I'm not sure the result is better though? GBFSData__1.java vs vehicle_status.GBFSData.java . On top of that it makes it much more cumbersome to migrate, as a simple search/replace of the package path now is turned into having to rename class name references, and in many cases figure out which of the __N is the right one.

testower commented 4 months ago

Can we please change this back?

Alessandro100 commented 4 months ago

After more investigation and playing with the imports of both I see the positives of having the file name in the import. I will work on this today. One thing I did notice would be colliding names 

import org.entur.gbfs.v2_2.vehicle_types.GBFSData;
import org.entur.gbfs.v2_2.system_alerts.GBFSData;
import org.entur.gbfs.v3_0.vehicle_types.GBFSData;

I'm curious if this is a common use case you come across, and if you see any solutions? I know import aliasing is not supported in Java but is in Groovy and some other jvm languages

testower commented 4 months ago

Colliding names are not problematic, that's exactly what package namespacing solves (among other things).

Here's what I propose:

  1. Revert back to the original generated code (from when you copied the repo). This ensures a predictable non-breaking migration path between the packages, requiring only to change the package path prefix from org.entur.gbfs to org.mobilitydata.gbfs.

  2. Continue discussing potential improvements for future releases.

I should be the first to admit that there is definitely room for improvement, since I have spent quite some time writing code using this library. But changes like this should be considered carefully because of the impact on existing code, and mixing them with a package migration path does not lead to a great experience.

Alessandro100 commented 4 months ago

Ok good to hear about the colliding name!

  1. The only modifications made from the original repo was the way schemas were gotten and handled. This was modified to use the local gbfs schemas. I made a PR #117 that corrects the import names, this should give you the desired functionality of simply changing org.entur to org.mobilitydata

  2. agreed