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

Change header value reading code to use enum constants and lambdas #57

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

dwalluck
Copy link
Contributor

@dwalluck dwalluck commented Apr 3, 2024

Instead of using magic constants in HeaderValue, use the types already defined in Type.

@dwalluck dwalluck requested a review from ctron April 3, 2024 22:33
@ctron
Copy link
Contributor

ctron commented Apr 4, 2024

I like that change. But do we still need that int field? Wouldn't it make sense to remove that?

@dwalluck
Copy link
Contributor Author

dwalluck commented Apr 4, 2024

I like that change. But do we still need that int field? Wouldn't it make sense to remove that?

We might not need it. In that case, I would just change type from int to Type and not add a new field.

But, how do we handle an unknown type? In the current code, it will throw an exception when parsing.

We can add an UNKNOWN enum, I guess. We have the UNKNOWN class there was well, but I think we have to modify it to hold the original type value int since the enum cannot hold it.

@ctron
Copy link
Contributor

ctron commented Apr 4, 2024

Yea, if we want to handle unknown ones, then we need a different strategy (I do miss Rust here 😁).

Maybe an interface, which gets implemented by both the enum, as well as Custom class (carrying the int).

@dwalluck
Copy link
Contributor Author

dwalluck commented Apr 4, 2024

Maybe an interface, which gets implemented by both the enum, as well as Custom class (carrying the int).

What is the goal, to hide the original raw value from the API?

Take a look at how I implemented it now. I stored the type inside the Unknown class.

If this.type == Type.UNKNOWN, then you must go through ((Unknown) this.value).getType() to get the original type.

@dwalluck
Copy link
Contributor Author

dwalluck commented Apr 4, 2024

If this.type == Type.UNKNOWN, then you must go through ((Unknown) this.value).getType() to get the original type.

Actually, we may be able to get rid of Unknown and set this.value = originalType. I thought we also had the actual binary data belonging to the unknown type that we needed to store, or is that not possible?

@dwalluck dwalluck force-pushed the header-values branch 2 times, most recently from 06e6f75 to c096b1f Compare April 5, 2024 14:42
@dwalluck
Copy link
Contributor Author

dwalluck commented Apr 5, 2024

@ctron OK, the Unknown class now should store the raw int type and the raw byte[] value.

Instead of using magic constants in `HeaderValue`, use the types already defined in `Type`.
@dwalluck dwalluck merged commit c367ac1 into eclipse:master Apr 10, 2024
4 checks passed
@dwalluck dwalluck deleted the header-values branch April 10, 2024 14:22
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