search

metafacture / metafacture-core

Core package of the Metafacture tool suite for metadata processing.
https://metafacture.org
Apache License 2.0
69 stars 34 forks source link

Enable generic header support in `HttpOpener`. #459

Closed blackwinter closed 2 years ago

blackwinter commented 2 years ago

Resolves #456.

blackwinter commented 2 years ago

accept-charset is extracted to a constant, accept is not

Only because we need to both set and get it.

setHeader(String,String) lowercases the key, setHeader(String) does not

The latter delegates to the former, so it's effectively the same.

blackwinter commented 2 years ago

We could discuss, however, whether trim() should be moved from setHeader(String) to setHeader(String, String). That's certainly an inconsistency of sorts. Or should we even drop it entirely?

blackwinter commented 2 years ago

accept-charset is extracted to a constant, accept is not

Thanks so much for mentioning this! Even though I wasn't going to at first, I went looking again and decided to extract accept into a constant as well. Which then lead to also extracting the default values - and me noticing that I had them swapped by mistake! Oops ;)

fsteeg commented 2 years ago

We could discuss, however, whether trim() should be moved from setHeader(String) to setHeader(String, String). That's certainly an inconsistency of sorts. Or should we even drop it entirely?

Yes, good point. I don't think we should drop it, I think its reasonable to set it like key: val, or even key : val – which would require the key to be trimmed too. So how about trimming both key and value, in setHeader(String, String)?

blackwinter commented 2 years ago

I think its reasonable to set it like key: val, or even key : val

Hm, I was assuming that user-supplied values would have to be standards-compliant. I was only trimming the value for internal consistency (setHeader("key: value") <=> setHeader("key", "value")).

fsteeg commented 2 years ago

Hm, I was assuming that user-supplied values would have to be standards-compliant. I was only trimming the value for internal consistency [...]

Alright, that makes sense, so the current implementation is good (but the value trimming is actually required).

blackwinter commented 2 years ago

but the value trimming is actually required

Can you be more specific? Whitespace around the field value is optional, so we're free to do either, aren't we?

fsteeg commented 2 years ago

Can you be more specific? Whitespace around the field value is optional, so we're free to do either, aren't we?

Oh, because we're just relaying the headers, and the actual server will handle the whitespace if we don't, yes, right!

blackwinter commented 2 years ago

Okay, then we'll just go with the current implementation :) Thanks!