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

Remove VALUE=DATE-TIME from DTSTART, DTEND, DTSTAMP #450

Merged

Conversation

niccokunzmann
Copy link
Member

This adds tests for #318.

If you like to fix them - please check out the branch issue-318-skip-value-parameter-for-default-date-time add some code and create a pull request to issue-318-skip-value-parameter-for-default-date-time.

@niccokunzmann niccokunzmann added tested and needs fix This pull request tests code but needs a contribution to fix the test - TDD hacktoberfest Issues for participation in the hacktoberfest https://hacktoberfest.com/ labels Oct 13, 2022
@jacadzaca
Copy link
Collaborator

@niccokunzmann will you add a changelog?

@jacadzaca jacadzaca linked an issue Oct 29, 2022 that may be closed by this pull request
@niccokunzmann
Copy link
Member Author

CHANGELOG. Ok, is a todo.

The communicative purpose of the plone tests is to notify the plone developers about potential problems of a certain release. Other projects use version fixing. The plone tests intoduce a new dimension of knowledge that is required to make icalendar green but not required to make it work.
I have three projects depending on icalendar. I cannot ask icalendar developers to engage with my tests.

I would like to ship this to master so that they can fix their tests fast.

@jacadzaca
Copy link
Collaborator

jacadzaca commented Oct 29, 2022

I think we shouldn't break peoples' tests on the weekend, but it's up to you

@niccokunzmann
Copy link
Member Author

It will take a while until I have the changelog. Tests broken is not production code broken. If it is, it must be changed.

Making progress here dependent on the plone tests and the other 1000 projects for that matter that use Icalendar, introduces a complexity that will kill this project - I guess - again.

I do not merge it now but would like to propose a process where we notify plone on release about breaking tests. They will use version-fixing icalendar==5.0.3 for example. Dependabot will bring updates, then.

@niccokunzmann
Copy link
Member Author

Looking at the tests, only their tests need changing.

@niccokunzmann
Copy link
Member Author

niccokunzmann commented Nov 2, 2022

@mauritsvanrees The plone tests are failing on this one. They require simple changes. What is the process you propose to update this? I would merge to master and then, a PR on the events repository can verify that the master is green?

In #447 (comment) you said: do not release. Production code would pin the version down when shipped - do you do that? Also: If I merge this and follow your request, then the other changes do not ger released. This is problematic. I do not like this process and would value some clarity around this. I think it is controversial in this community to just ship either way, so I ask with a bit of patience. The current process slows us down. What can we do to improve it? See also the comment above.

Also, it is only the tests failing. The production environment will still work.
I would really value your input on this.

mauritsvanrees added a commit to plone/plone.app.event that referenced this pull request Nov 2, 2022
There ``;VALUE=DATE-TIME`` is not included because it is already the default.
See collective/icalendar#450
@mauritsvanrees mauritsvanrees changed the title add tests for removing VALUE=DATE-TIME from DTSTART, DTEND, DTSTAMP Remove VALUE=DATE-TIME from DTSTART, DTEND, DTSTAMP Nov 2, 2022
@mauritsvanrees
Copy link
Member

I have updated the title of this PR. It does not only add tests, but it has the actual fix, right?

@mauritsvanrees The plone tests are failing on this one. They require simple changes. What is the process you propose to update this? I would merge to master and then, a PR on the events repository can verify that the master is green?

In my local setup I have cloned icalendar with this PR branch and ran the plone.app.event tests and then I see they fail. So I can test this without being merged to master.

In #447 (comment) you said: do not release. (...) The current process slows us down. What can we do to improve it?

In that same comment I said, in case these tests go red "you can ping me". You did not do that until now, so I did not know about this yet.

I have created a PR to fix the plone.app.event tests to work both with the old and new code. I don't think it is too much to ask to wait for that.

I do not want to hold up icalendar changes, so If I am unresponsive it is fine to merge and release anyway. But so far I don't think this is the case.

mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Nov 2, 2022
Branch: refs/heads/master
Date: 2022-11-02T10:18:38+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.event@92383d4

Fixed tests to work with future icalendar.

There ``;VALUE=DATE-TIME`` is not included because it is already the default.
See collective/icalendar#450

Files changed:
A news/450.bugfix
M plone/app/event/tests/test_icalendar.py
Repository: plone.app.event

Branch: refs/heads/master
Date: 2022-11-02T11:09:04+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.event@8a69f0d

Merge pull request #361 from plone/maurits-icalendar-tests

Fixed tests to work with future icalendar.

Files changed:
A news/450.bugfix
M plone/app/event/tests/test_icalendar.py
@mauritsvanrees
Copy link
Member

I have merged my plone.app.event PR and am now rerunning the failed job here.

@mauritsvanrees
Copy link
Member

Green!
Only a changelog entry is missing now.

@niccokunzmann
Copy link
Member Author

Merged and CHANGELOG is modified.

@mauritsvanrees mauritsvanrees merged commit 5dae2a4 into master Nov 3, 2022
@mauritsvanrees mauritsvanrees deleted the issue-318-skip-value-parameter-for-default-date-time branch November 3, 2022 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Issues for participation in the hacktoberfest https://hacktoberfest.com/ tested and needs fix This pull request tests code but needs a contribution to fix the test - TDD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip VALUE properties when using the default value
3 participants