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

Prefix mailto: for email in organizer property #88

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

meain
Copy link
Contributor

@meain meain commented Feb 27, 2024

Organizer property should be similar to how attendee is handled.

golang-ical/components.go

Lines 260 to 262 in 0fcebed

func (cb *ComponentBase) AddAttendee(s string, props ...PropertyParameter) {
cb.AddProperty(ComponentPropertyAttendee, "mailto:"+s, props...)
}

arran4
arran4 previously approved these changes Feb 27, 2024
Copy link
Owner

@arran4 arran4 left a comment

Choose a reason for hiding this comment

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

LGTM

@arran4
Copy link
Owner

arran4 commented Feb 27, 2024

Agreed. Consistency is king, also the spec is very clear on this too: https://www.kanzaki.com/docs/ical/calAddress.html I'm not sure if it can be something else.

This might break some existing deployments. I will get back to this, in a couple days.

@arran4
Copy link
Owner

arran4 commented Mar 4, 2024

Need to delay, interview prep.

@meain
Copy link
Contributor Author

meain commented Mar 4, 2024

I don't know if this is "clean", but if you have concerns about the user sending in email with "mailto:" prefix, I think we can probably consider only conditionally adding it by checking the prefix.

@arran4
Copy link
Owner

arran4 commented Mar 4, 2024

It makes sense and if there is an issue someone can submit an issue and we can add a new code path. There is almost no need for duplication. : is not part of dot-atom https://datatracker.ietf.org/doc/html/rfc2822#section-3.2.4 so I think we are home free with that one.

@meain
Copy link
Contributor Author

meain commented Mar 4, 2024

Sounds good, I've updated it to only conditionally add it. Also updated the attendee one to be similar. Let me know if I should revert how attendee works to previous version.

Copy link
Owner

@arran4 arran4 left a comment

Choose a reason for hiding this comment

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

mailto: is capitalized in the rfc might as well stick to that too. -- If there is a string const use that?

@arran4
Copy link
Owner

arran4 commented Mar 4, 2024

But other than that looks good.

@meain
Copy link
Contributor Author

meain commented Mar 4, 2024

Spent a bit more time looking into the spec. Looks like it is possible to have prefixes other than "mailto:" here. A CAL-ADDRESS can be a URI and if you see the doc, a URI can be more than just mailto. I think it might be incorrect to automatically add mailto: prefix after all 😅 . For my personal use-case, I'll always need the mailto: prefix and so I am fine either way. I'll let you decide how we proceed here.


mailto: is capitalized in the rfc might as well stick to that too.

I see the examples in the RFC to be using lowercase. I also see that there has been a previous discussion about this here.

If there is a string const use that?

There is none defined as of now, but I can add one if we are certain about adding the mailto: prefix.

@arran4
Copy link
Owner

arran4 commented Mar 4, 2024

haha yep. I'm in a loop. Okay. Nothing more is necessary.

Copy link
Owner

@arran4 arran4 left a comment

Choose a reason for hiding this comment

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

LGTM

@arran4 arran4 merged commit 84a339a into arran4:master Mar 4, 2024
13 checks passed
@arran4
Copy link
Owner

arran4 commented Mar 4, 2024

Thanks for looking that up. I have a shocking memory it seems.

There you go:

https://github.com/arran4/golang-ical/releases/tag/v0.2.7

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