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

Use UUIDs for Event UIDs #203

Closed
janrg opened this issue Jul 27, 2020 · 9 comments
Closed

Use UUIDs for Event UIDs #203

janrg opened this issue Jul 27, 2020 · 9 comments

Comments

@janrg
Copy link
Contributor

janrg commented Jul 27, 2020

As discussed in #200, according to https://icalendar.org/New-Properties-for-iCalendar-RFC-7986/5-3-uid-property.html, the UID property should preferably be a UUID and should not include any domain information. This will also make the domain attribute obsolete.

@gprst
Copy link

gprst commented Sep 28, 2020

Hi ! I am currently working on this. I've chosen uuid-random for the UUID generation because it seems to be UUIDv4-complient, and be quite light.

I've noticed the domain() function has been deprecated for quite a long time (since 0.3.0, current version is 1.15.0), so I'm not quite sure if I should just delete the property at once, or just leave it as "deprecated" ?

@sebbo2002
Copy link
Owner

Please leave it as deprecated for now, I would not like to open a new major version because of such a small change. It would be an incompatible API change in any case. That would be a point I would want to address in version 2.x.

@sebbo2002
Copy link
Owner

Ah sorry, read it again and noticed that I am probably not quite awake yet. You can knock out domain(), but I wouldn't merge the PR until there is a bit more accumulated for a version 2.0. Hope that is okay for you then.

@gprst
Copy link

gprst commented Sep 29, 2020

I could still push two PRs, one for the UUID (that I would actually need haha, your project's great and I'm using it), and one for the removal of domain(). Note that this would also include the removal of all occurences of the domain property accross the examples ; I've already started, and changed domain for name when domain was just used as an example property.

@gprst
Copy link

gprst commented Sep 29, 2020

Also I will push the PR in a couple of days haha, I want my Hacktoberfest t-shirt 😅

@sebbo2002
Copy link
Owner

I think two pull requests are a good solution. Then I can merge one directly if the solution is backwards compatible. The removal of domain() would then be put off to 2.0. Also you are then a bit further on the t-shirt with two pull requests instead of one. 😂

@gprst
Copy link

gprst commented Oct 3, 2020

I ended up using the same PR for the whole issue, with two commits : one for the UUID part, the other for the domain property removal 🙂 I hope it looks like what you expected.

@sebbo2002
Copy link
Owner

@gprst Thanks, this will definitely be in version 2.0.

@sebbo2002
Copy link
Owner

Closing as we now have #215.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants