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 support for RFC2369 #171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tristan957
Copy link

Exposes parsed URLs from the various list commands described in RFC2369.

@tristan957
Copy link
Author

tristan957 commented Aug 4, 2023

@emersion I have a strong feeling my parsing code is not that great, so please keep me honest :).

I would really like some variant of this PR to go in to add a feature to aerc.

Exposes parsed URLs from the various list commands described in RFC2369.
Comment on lines +220 to +223
// Consume a potential newline + indent.
p.consume('\r')
p.consume('\n')
p.skipSpace()
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 the input values ever contain newlines. These are stripped by the header parser when unfolding the header fields.

Comment on lines +237 to +240
i := 0
for p.s[i] != '>' && i+1 < len(p.s) {
i += 1
}
Copy link
Owner

Choose a reason for hiding this comment

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

Probably can be simplified with strings.IndexByte?

var lit string
lit, p.s = p.s[:i], p.s[i:]

u, err := url.Parse(lit)
Copy link
Owner

Choose a reason for hiding this comment

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

The RFC recommends that we remove any whitespace character from the string in-between the angle brackets.

// This can be used on List-Help, List-Unsubscribe, List-Subscribe, List-Post,
// List-Owner, and List-Archive headers.
//
// See https://www.rfc-editor.org/rfc/rfc2369 for more information.
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: we can just use plaintext to reference the RFC: "See RFC 2369". GoDoc will automatically linkify it, and it's more readable.


var ids []string
for _, url := range urls {
ids = append(ids, url.String())
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we could handle url == nil, and write "NO"?

@@ -362,6 +447,18 @@ func (h *Header) SetMsgIDList(key string, l []string) {
}
}

func (h *Header) SetListCommandURLList(key string, urls []*url.URL) {
Copy link
Owner

Choose a reason for hiding this comment

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

Would be nice to document this method.

@@ -362,6 +447,18 @@ func (h *Header) SetMsgIDList(key string, l []string) {
}
}

func (h *Header) SetListCommandURLList(key string, urls []*url.URL) {
if len(urls) == 0 {
h.Del(key)
Copy link
Owner

Choose a reason for hiding this comment

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

We need to return here, or else we will later on set the header to <>.

//
// In the case that the value of List-Post is the special value, "NO", the
// return value is a slice containing one element, nil.
func (h *Header) ListCommandURLList(key string) ([]*url.URL, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should just use the name ListCommand, and leave out the URLList part.

@emersion
Copy link
Owner

Hey @tristan957, any chance you'd have some time to update this PR?

@tristan957
Copy link
Author

@emersion yeah sorry. Let me get back to it this week!

@emersion
Copy link
Owner

emersion commented Feb 1, 2024

Gentle ping, just in case you happen to have time on your hands :)

@tristan957
Copy link
Author

I know. I've been reminding myself to come back to it :(. Were you hoping to get this in for a release or something? Asking if I need to set a deadline for myself.

@emersion
Copy link
Owner

emersion commented Feb 2, 2024

Oh, no rush, I don't need this for any release in particular, just making sure this isn't getting forgotten :P

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