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

Remove static methods in initializr-metadata's public API that are only used in tests #956

Open
wilkinsona opened this issue Jul 14, 2019 · 4 comments
Assignees

Comments

@wilkinsona
Copy link
Contributor

The initializr-metadata module has some public API that is only used in tests. It'd be nice to reduce the public API and pull those methods out into something that is test-specific. That might be a -test-support module similar to what we have in Spring Boot.

Some methods that are candidates for removal from the public API:

  • io.spring.initializr.metadata.BillOfMaterials.create(String, String)
  • io.spring.initializr.metadata.BillOfMaterials.create(String, String, String)
  • io.spring.initializr.metadata.BillOfMaterials.Mapping.create(String, String)
  • io.spring.initializr.metadata.BillOfMaterials.Mapping.create(String, String, String...)
  • io.spring.initializr.metadata.Dependency.create(String, String, String, String)
  • io.spring.initializr.metadata.Dependency.withId(String, String)
  • io.spring.initializr.metadata.Dependency.withId(String, String, String, String)
  • io.spring.initializr.metadata.Dependency.withId(String, String, String, String, String)
  • io.spring.initializr.metadata.Dependency.Mapping.create(String, String, String, String, Boolean)

Of the methods listed above, one of them is used by tests in start.spring.io. We could consider a published test module or copy and paste as the usage is pretty minimal.

There's also a mixture of overloading (Dependency.withId) and methods that may be called with several nulls (Dependency.Mapping.create). It'd be nice to be consistent and I wonder if a builder-based approach might work quite well here.

@wilkinsona wilkinsona added this to the 0.8.0 milestone Jul 14, 2019
@wilkinsona wilkinsona self-assigned this Jul 14, 2019
@snicoll
Copy link
Contributor

snicoll commented Jul 14, 2019

For the record, this was introduced when we migrated from Groovy to Java and such "overloaded constructors" were given to us for free.

initializr-metadata is the only module that we haven't really touched when we refactored the backend API. It still has the notion of bootVersions that I'd like to harmonize with platformVersion. There is also the mutable model and the mixture between configuration and metadata. Moving to a builder-based approach was one of the goal I had in mind for this so thanks for initiating the work!

@wilkinsona
Copy link
Contributor Author

wilkinsona commented Jul 14, 2019

Thanks for the background info. Good to hear this sort of thing is heading in the right direction.

Bom creation could look something like this perhaps:

BillOfMaterials fooBom = MetadataBuilder.bomWithCoordinates("org.foo", "bom",
        (bom) -> bom
                .mappingWithRange("[1.0.0.RELEASE, 1.1.8.RELEASE)", (mapTo) -> mapTo.version("1.0.0.RELEASE"))
                .mappingWithRange("1.1.8.RELEASE", (mapTo) -> mapTo.version("2.0.0.RELEASE")));

@wilkinsona
Copy link
Contributor Author

And a slightly more complex example:

BillOfMaterials exampleBom = MetadataBuilder.bomWithCoordinates("com.example", "bom", (bom) -> bom
        .version("1.0.0").versionProperty("bom.version").repository("repo-main").additionalBom("bom-main")
        .mappingWithRange("[1.2.0.RELEASE,1.3.0.M1)",
                (mapTo) -> mapTo.version("1.1.0").groupId("com.example.override").artifactId("bom-override")));

@snicoll
Copy link
Contributor

snicoll commented Jul 15, 2019

Interesting, thank you. One more thing to consider before spending efforts on this.

If we keep a @ConfigurationProperites-based arrangement, It is likely that we'll have two completely separate model with a BillOfMaterialsConfiguration that provides the model that one case use to configure and the existing BillOfMaterials being a immutable/resolved/richer model that the rest of the infrastructure will use for code generation. Worth taking that idea in account here IMO.

@snicoll snicoll removed this from the 0.8.0 milestone Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants