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

Parser and Assembler refactoring #31

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

swismer
Copy link
Collaborator

@swismer swismer commented May 26, 2023

If you want to have a closer look, I would suggest to review each commit individually.
Some comments:

  • I hope it's OK to drop support for Java 7. I mainly wanted to use default methods instead of introducing an AbstractReader. BTW, what about updating the Java versions used in the GitHub actions? 9-13 could be replaced by the LTS versions 8, 11 and 17...
  • While moving code around, I tried to change as few things as possible, as this is difficult to review. However, I cleaned up cases where both a byte array and a DataReader (on the same data) have been passed as parameter.
  • As I'm using this library as maven dependency and creating such PRs to get rid of my local patches, I would be glad, if you could deploy a release some day... :-)

On my TODO list for upcoming PRs (in case you want to comment on those):

  • Do more cleanups: Remove empty classes, separate unit tests from experiments (and remove some of them), remove unused test resources
  • Provide functions to recalculate sizes/offsets/pointers/checksums etc. - basically the first step to modify parts of an executable.

@kichik
Copy link
Owner

kichik commented May 29, 2023

Thanks, I'll read.

  • No objection Java 7 removal. It's EOL AFAIK.
  • We should update GitHub Actions to at least add 8 and 17. I wouldn't want to remove any versions. The more versions tested, the better.
  • As I'm using this library as maven dependency and creating such PRs to get rid of my local patches, I would be glad, if you could deploy a release some day... :-)

0.3.3 was released 3 weeks ago with #29. Maven is automated once I create a tag. The GitHub release needs to be published which I forgot to do.

@swismer
Copy link
Collaborator Author

swismer commented May 30, 2023

Thank you!
FYI: Java 9, 10, 12 and 13 are EOL as well. If more is better: Maybe use all the versions currently maintained by Adoptium Temurin (8, 11, 16-20)?

@kichik
Copy link
Owner

kichik commented Jun 26, 2023

That's a lot of deprecations 😅

@kichik kichik merged commit a95ee6e into kichik:master Jun 26, 2023
@kichik
Copy link
Owner

kichik commented Jun 26, 2023

Thank you! FYI: Java 9, 10, 12 and 13 are EOL as well. If more is better: Maybe use all the versions currently maintained by Adoptium Temurin (8, 11, 16-20)?

That sounds good.

@swismer swismer deleted the parser-assembler-refactoring branch June 27, 2023 20:07
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