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

iss41: notes on issues and undecodable response #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

a2800276
Copy link

@a2800276 a2800276 commented Nov 16, 2024

This PR solves the issue described in #41.

I want to try to keep this PR smallish in order to make it more manageable:

  • this only touches Response, I assume there are symmetrical issues in Request ( not sure if Req and Resp even need to be different structs...)
  • there are some architectural issues I don't understand, e.g. why are are the OperationalAttributes in Response a single Attributes, while the Job- and PrinterAttributes are arrays of Attributes. This doesn't seem necessary and adds a lot of code to check for nil and initialize maps and arrays.
  • I've added UnsupportedAttributes to the response, but I am not sure if this is a complete list of AttributeGroups. BTW I added UnsupportedAttributes as an array, like Printer and Job Attributes (for no particular reason, just to stay consistant with existing code)
  • Attribute values are currently decoded into byte arrays. The Value field of Attribute have an interface{}type. I'm not sure if it would be desirable to cast them into something more concrete by value tag (RFC 8010 3.5.1), but in that case, we could also consider using Generics here? Again, bigger change I didn't want to just go ahead with.
  • I left the SkipDecoding tag set to true in the Response tests. The decoding routine I wrote seems to work fine and I have another test for it. But the samples in response_test.go are a little wonky and I was too lazy to figure out how they are supposed to work. I think it's an issue with the Encoder adding default attributes even if they weren't explicitly set in the structs?

It looks like some major refactoring might be in order. Maybe utilizing OpenPrinting's Go library for encoding and decoding. Again, this would be a major change in the architecture and I wanted to keep the changes to review manageable.

Anyway, thanks for helping out! Let me know if you have any questions.

@a2800276 a2800276 force-pushed the iss_41_response_decoder branch from 7851a16 to fdbedad Compare December 9, 2024 08:09
@phin1x
Copy link
Owner

phin1x commented Dec 13, 2024

hi @a2800276

Attribute values are currently decoded into byte arrays. The Value field of Attribute have an interface{}type. I'm not sure if it would be desirable to cast them into something more concrete by value tag (RFC 8010 3.5.1), but in that case, we could also consider using Generics here? Again, bigger change I didn't want to just go ahead with.

the code was originally written for go1.12 when generics were still a long way off. if you want, you can add generics to the library. however, this should be in a separate pr.

there are some architectural issues I don't understand, e.g. why are are the OperationalAttributes in Response a single Attributes, while the Job- and PrinterAttributes are arrays of Attributes. This doesn't seem necessary and adds a lot of code to check for nil and initialize maps and arrays.

i don't remember why i decided to use an array. when i look at the code today i don't see why an array should be used. i think we can make a breaking change here and remove the array.

@a2800276
Copy link
Author

Just so this doesn't get stalled indefinitely: not sure if this was an intermediate quick answer to my questions concerning generics and are still reviewing or whether you are expecting more input from me?

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