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

Case insensitive rfc822 keys parsing and setting. #107

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

Conversation

ebikt
Copy link
Contributor

@ebikt ebikt commented May 29, 2019

WARNING: This change breaks API of Paragraph, because indexing using [] is no longer possible.
paragraph.Values is replaced by paragraph.Set(key, value), paragraph.Get(key) and paragraph.Has(key) (latter two are also combined to paragraph.Get2(key)). These functions may need inline documentation (not provided)).

Paragraph structure now consists of .Order which is case-sentisitve and mapping .values which is case insensitive. Idea is to keep upper and lower characters for serializing same as in some .Set(...) call for given key, because case rules are ad-hoc and we would have to keep (pluggable) dictionary of cannonical spelling of keys. (Typically hash names are spellt in different ways.) Current implementation uses spelling of first .Set(...) call. Spelling change of key is now not supported, although one can directly edit .Order. Maybe some API for correcting spelling should be provided.

This change is done to support #57

p.values[k] = value
}

func (p Paragraph) Get2(key string) (string, bool) {
Copy link
Owner

Choose a reason for hiding this comment

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

The name Get2 is funny. Maybe let's only provide this form of Get, and other uses can do value, _ = p.Get or make their own wrapper. If we want to add the other one later, that seems like the special case (and maybe has a better name GetOrEmpty or something). Let's punt on that for now.

Copy link
Owner

Choose a reason for hiding this comment

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

Also maybe we want to run strings.Lower on the input here. The user may not know about the case-insensitive nature of 2822

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We run strings.ToLower in slow-path. Hmm, there is missing piece of documentation, that describes using lowercase keys for Get as "fast-path". If key is not found then it is lowercased and tried again.
I wrote this by guessing strings.ToLower(key) to be much slower operation than p.values[key]

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need to worry about speed - it's going to add quite a bit to reason about and mental overhead. Let's lower all keys before looking up, we could leave Values exported and tell people to not touch if speed really matters to them. Or perhaps a way to get a copy of the map.

@paultag
Copy link
Owner

paultag commented May 30, 2019

Comments inline, thanks for your patches!

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.

None yet

3 participants