-
Notifications
You must be signed in to change notification settings - Fork 6
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
Switch from Bintray to JitPack. Fixes #14 #15
Conversation
Strange, it seems some trouble with JitPack -- raml-org/webapi-parser#92 -- although locally it built without problems. |
pom.xml
Outdated
<version>0.8</version> | ||
<version>0.9</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>io.kaitai</groupId> | ||
<artifactId>kaitai-struct-compiler_2.12</artifactId> | ||
<version>0.8</version> | ||
<version>0.9</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to see how this KS version change is related to the PR topic (i.e. switching from one package repository to another). I see that you did this in hope that it might fix the CI failure:
1766eba - Update KaitaiStruct compiler and runtime to 0.9 - may be this fix JitPack error...
but it apparently didn't do anything about that, because the CI failed with exactly the same error as the commit before. It's unlikely that this would be a requirement for the Bintray->JitPack transition, so extract that commit to another PR. Packing unrelated changes to a single PR quickly becomes a mess: please don't do that.
If the 0.8->0.9 and Bintray->JitPack tasks were somehow connected and wouldn't make sense to merge one without the other (which is not the case here I believe), you should at least state the 0.8->0.9 change in the PR title and describe in the initial PR comment why you're doing that (i.e. "I had to update Kaitai Struct compiler + Java runtime from 0.8 to 0.9 because otherwise the Bintray->JitPack transition wouldn't be possible."). Right now the PR title says only "Switch from Bintray to JitPack. Fixes #14", from which is not clear that you included this separate (quite major) change, and there is no justification for that. It may be buried somewhere in the commit messages, but that's not enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It need to upgrade dependencies anyway. Extracted to #16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks! Merging in.
Holy goddess! Maybe you can push some other merge buttons :) |
Found an interesting site which makes maven artifacts from the GitHub releases -- https://jitpack.io/#Mingun/JHexView/v2.1
This should fix #14.