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

Add HeaderDecoder with custom header name matching #316

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

Conversation

paulpdaniels
Copy link
Contributor

Closes #315

Adds a new set of decoderWith operators to the HeaderDecoder and HeaderCodec companion objects. The old method decoder method now delegates to decoderWith but uses the default equality check for strings.

This allows users to override how the strings are compared when trying to determine the order of the headers in the mapping, e.g. when comparing the strings in a case insensitive manner.

@nrinaudo
Copy link
Owner

Right, this is really something I should put in the contribution guidelines. There's no issue that I can see with your PR, aside from a purely cosmetic one: the code is not formatted according to the project's guidelines.

You just need to run the scalafmtAll SBT task and it'll fix the problem(s).

@nrinaudo
Copy link
Owner

Also: given that your use case is likely quite a common one, I wouldn't be against pre-defined helpers. We have one for case-sensitive headers, why not:

  [#
  def caseInsensitiveDecoder[[#A1: CellDecoder#], R]([#f1: String#])(f: ([#A1#]) => R): HeaderDecoder[R] =
    decoderWith([#f1#])(_.equalsIgnoreCase(_))(f)#
  ]

@paulpdaniels
Copy link
Contributor Author

@nrinaudo comments addressed

@nrinaudo
Copy link
Owner

I, err... were they? I'm not as used to the Github UI as I should be, but checking out the latest version of the branch locally still has }} rather than }}} in GeneratedHeaderDecoders, which triggers compile time warnings (and probably disable some doctests, although I haven't checked).

@paulpdaniels
Copy link
Contributor Author

Ah ok, I switched computers because scalafmt on my windows was going crazy, so I must have lost that change. Will fix

@paulpdaniels
Copy link
Contributor Author

Ok I think they should be fixed now 🤞

@paulpdaniels
Copy link
Contributor Author

Apologies I lost track of this and #319 until I got the notification this morning. Anyway I think I addressed your comments @nrinaudo let me know if there is anything else you want on this.

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.

Case insensitive headers
2 participants