BlueObelisk / chemicaltagger

ChemicalTagger is a tool for semantic text-mining in chemistry.
Apache License 2.0
36 stars 8 forks source link

2.0.0 release and possible maven coordinate changes? #2

Closed mjw99 closed 4 years ago

mjw99 commented 4 years ago

I am trying to fix and have a working compile for chemicaltagger-webapp. This depends on acpgeo, which in turn, depends on chemicaltagger.

I have been able to fix various issues with acpgeo's ANTLR grammar compilation, specifically with Dan Lowe's (@dan2097) help and guidance; these currently exist on a fork here. Dan's fix has also required a change in chemicaltagger, hence the need to release a new version of chemicaltagger, so that acpgeo will compile.

(As an aside, chemicaltagger-webapp is very nearly there on this fork here.)

So, the first issue is, should I release a 1.5.0 or a 2.0.0 of chemicaltagger? My feeling here is that we should go for a 2.0.0 since the minimal version of java is now 1.8 as a result of an updated parent pom, however, commons-io only changed a minor for something similar.

Second, I've noticed a long standing issue in this project's pom; essentially, an artifactId should be all lower case and currently it is "chemicalTagger". This actually matters in dependency resolution and I had been fighting some strangeness with acego's mixed case artifactId with some local SNAPSHOTS. Should we fix this now in the new release?

Third, should the groupId for this be uk.ac.cam.ch.wwmm.chemicaltagger and not has it currently is, i.e. uk.ac.cam.ch.wwmm ? This would be more like oscar's coordinates.

petermr commented 4 years ago

I don't have any contrary indications so seems OK to me.

On Mon, Jan 20, 2020 at 10:02 PM Mark J. Williamson < notifications@github.com> wrote:

I am trying to fix and have a working compile for chemicaltagger-webapp https://github.com/BlueObelisk/chemicaltagger-webapp. This depends on acpgeo https://github.com/BlueObelisk/acpgeo, which in turn, depends on chemicaltagger.

I have been able to fix various issues with acpgeo's ANTLR grammar compilation, specifically with Dan Lowe's (@dan2097 https://github.com/dan2097) help and guidance; these currently exist on a fork here https://github.com/mjw99/acpgeo/tree/ftbfs_fixes. Dan's fix has also required a change https://github.com/BlueObelisk/chemicaltagger/commit/40cd4e8a735ad1e522dc4f12f974aa76195c6248 in chemicaltagger, hence the need to release a new version of chemicaltagger, so that acpgeo will compile.

(As an aside, chemicaltagger-webapp is very nearly there on this fork here https://github.com/mjw99/chemicaltagger-webapp/tree/ftbfs_fixes.)

So, the first issue is, should I release a 1.5.0 or a 2.0.0 of chemicaltagger? My feeling here is that we should go for a 2.0.0 since the minimal version of java is now 1.8 as a result of an updated parent pom, however, commons-io only changed a minor for something similar https://commons.apache.org/proper/commons-io/upgradeto2_6.html.

Second, I've noticed a long standing issue in this project's pom; essentially, an artifactId should be all lower case https://maven.apache.org/guides/mini/guide-naming-conventions.html and currently it is "chemicalTagger". This actually matters in dependency resolution and I had been fighting some strangeness with acego's mixed case artifactId with some local SNAPSHOTS. Should we fix this now in the new release?

Third, should the groupId for this be uk.ac.cam.ch.wwmm.chemicaltagger and not has it currently is, i.e. uk.ac.cam.ch.wwmm ? This would be more like oscar https://search.maven.org/search?q=g:uk.ac.cam.ch.wwmm.oscar's coordinates.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/BlueObelisk/chemicaltagger/issues/2?email_source=notifications&email_token=AAFTCS3CNXFAGP5KZSEPJHLQ6YNO3A5CNFSM4KJKN7E2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IHO4RTQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFTCS3R6MAKS662HMVJSL3Q6YNO3ANCNFSM4KJKN7EQ .

-- Peter Murray-Rust Founder ContentMine.org and Reader Emeritus in Molecular Informatics Dept. Of Chemistry, University of Cambridge, CB2 1EW, UK

dan2097 commented 4 years ago

I slightly favour 1.5.0 if only because there are no breaking API changes, there are relatively few code changes, and practically all users are likely to be using Java8+ anyway. I have no strong feelings on the artifactId, there are pros and cons to changing it. I don't think the groupid needs to change:

A good way to determine the granularity of the groupId is to use the project structure. That is, if the current project is a multiple module project, it should append a new identifier to the parent's groupId

ChemicalTagger isn't a multi-module project (OSCAR 4 is). Admitedly I can see that this recommendation is a bit problematic if a project evolves into a multi-module project as has typically been my experience... Personally I think having the project in the groupid a bit redundant if all the artifactids are also prefixed with the project.

mjw99 commented 4 years ago

Thank you for all the input and advice here. I'm going to go for a 1.5.0 release and maintain the status quo.

mjw99 commented 4 years ago

1.5.0 has been deployed to Maven Central.