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

More saxon curations #263

Merged
merged 3 commits into from
Jan 20, 2025
Merged

More saxon curations #263

merged 3 commits into from
Jan 20, 2025

Conversation

sschuberth
Copy link
Member

No description provided.

@sschuberth sschuberth requested a review from a team as a code owner January 18, 2025 09:28
@sschuberth sschuberth enabled auto-merge (rebase) January 18, 2025 09:28
@sschuberth sschuberth changed the title More Saxon curations More saxon curations Jan 18, 2025
Copy link
Member

@tsteenbe tsteenbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understand why you would extract base properties for Saxon but the id/comment combination is incorrect as https://repo1.maven.org/maven2/net/sf/saxon/saxon/8.7/saxon-8.7.pom does include SCM tag. Recommend to update the comment to use Maven Central pom location but mention SCM url does not work anymore.

Copy link
Member

@tsteenbe tsteenbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to request 2 more changes:

  1. Drop the comment for id: Maven:net.sf.saxon:saxon:9 curation it mentionsthe POM in vcs revision curation - makes no sense to me.
  2. Improve the comment for Maven:net.sf.saxon:saxon curation to include POM links for 8.7 and 9 to as proof of correctness of curation.

Maybe change from:

- id: "Maven:net.sf.saxon:saxon"
  curations:
    comment: |-
      The POMs for the different versions either have no or an outdated SCM tag.

to

- id: "Maven:net.sf.saxon:saxon"
  curations:
    comment: |-
      The POMs for the different versions either have no or an outdated SCM tag.
      See https://repo1.maven.org/maven2/net/sf/saxon/saxon/8.7/ and
      https://nexus.intranda.com/repository/maven-releases/net/sf/saxon/saxon/9/saxon-9.pom.

@sschuberth
Copy link
Member Author

  1. it mentionsthe POM in vcs revision curation - makes no sense to me.

Why not? If the POM contained an scm tag, then no VCS curation would be needed. Also in the initial version you explicit requested the link to the POM to be added.

curation to include POM links for 8.7 and 9 to as proof of correctness of curation.

There are more version that those, and I don't think it makes sense to add each of them as a proof. So I'll pick just one as an example.

@sschuberth
Copy link
Member Author

Ok, I now get your point: You're arguing that now with the https://nexus.intranda.com/repository/maven-releases/net/sf/saxon/saxon/9/saxon-9.pom link being part of the more generic curation, it does not need to be repeated in the version-specific curation. I agree to that.

@sschuberth sschuberth merged commit ee52a6d into main Jan 20, 2025
2 checks passed
@sschuberth sschuberth deleted the more-saxon branch January 20, 2025 08:56
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.

2 participants