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 use of Object in headers and store header value as rpm tag v… #62

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

dwalluck
Copy link
Contributor

@dwalluck dwalluck commented Apr 6, 2024

…alue

An attempt at removing some duplicate code between RpmTagValue and RpmInformations.

Improve type safety by always storing a HeaderEntry in the header map instead of raw Object. This required a small API change to call getValue() now to get the actual value, and the stored value was changed to RpmTagValue instead of Object inside the HeaderEntry itself.

I think another possible improvement that is not in this pull request as it requires more code changes is to avoid returning Object from the get() API, and instead return an RpmTagValue and have the caller have to provide asLong() or asString() instead of casting, which may provide better type safety.

@dwalluck dwalluck requested a review from ctron April 6, 2024 15:48
Copy link
Contributor

@ctron ctron left a comment

Choose a reason for hiding this comment

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

Only a small thing.

@@ -118,19 +117,44 @@ public Optional<Long> asLong() {
return Optional.empty();
}

if (this.value instanceof Long) {
return Optional.of(((Long) this.value).longValue());
if (value instanceof Number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this change. First of all, it's not aligned with the other (int for example). But also because it won't work with the array variant.

One way to deal with this could be to add a asNumber variant. Which would still need some strategy for the array case I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ctron This code actually came from preexisting code in RpmInformations::asLong that I merged into RpmTagValue::asLong. I don't think I actually changed anything. However, I reverted all changes to this file now except for the absolute minimum, so please look again.

In general, I do not like automatically converting between Integer and Long and I'd like to get rid of it in a different pull request. Java 8 has Integer::toUnsignedLong which we can use for uint32 where needed, but this doesn't actually require changing the data type. We should just change it at the RpmInformations level, but I think we should make that an even higher level, i.e., store an Instant instead of a Long there.

@dwalluck dwalluck force-pushed the header-entry branch 2 times, most recently from 41c6bf5 to aa76291 Compare April 12, 2024 15:59
@dwalluck dwalluck merged commit 454e760 into eclipse:master Apr 12, 2024
4 checks passed
@dwalluck dwalluck deleted the header-entry branch April 12, 2024 18:02
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