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

Fix vevent location backcompat #201

Closed
wants to merge 0 commits into from

Conversation

gRegorLove
Copy link
Member

@gRegorLove gRegorLove commented Aug 25, 2018

Includes whitespace fixes in ClassicMicroformatsTest.php View updates sans whitespace.

Fixes #184

@gRegorLove gRegorLove requested review from Zegnat and aaronpk August 25, 2018 19:55
@gRegorLove gRegorLove requested a review from dshanske April 15, 2020 05:58
Copy link
Member

@dshanske dshanske left a comment

Choose a reason for hiding this comment

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

No issue here

@Zegnat
Copy link
Member

Zegnat commented May 2, 2020

According to the original issue #184 this tries to address two tests:

  1. hcalendar/combining: this one now passes. No more extra children property. location is correctly added with a nested h-card.
  2. hcalendar/attendees: this one does not pass yet. This PR now adds a location property, the test expect it to be the string San Francisco but the parser now makes it an h-card. If there is no vcard or adr class, none should be implied.

Mf2/Parser.php Outdated Show resolved Hide resolved
@gRegorLove
Copy link
Member Author

The Travis failure now is on PHP 5.5 with masterminds/html5. Once #220 is resolved I can rebase on master and that should be good to go.

Copy link
Member

@tantek tantek left a comment

Choose a reason for hiding this comment

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

The updated functional edits LGTM.

Nit: were the indentation/whitespace changes in ClassicMicroformatsTest.php really necessary?

I'm not happy about them (so many that GitHub didn't even display them by default and I had to expand the diffs!) but it's also not blocking. I'd prefer if those could be reverted so I'm marking this "Request changes". If there was a good reason to mix that into this commit, please comment accordingly and I'll approve.

@gRegorLove
Copy link
Member Author

The .editorconfig introduced a little bit ago specifies tabs so I think my editor did that automatically.

Adding ?w=1 to the URL removes the whitespace changes for reviewing: https://github.com/microformats/php-mf2/pull/201/files?w=1

Copy link
Member

@Zegnat Zegnat left a comment

Choose a reason for hiding this comment

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

I have rerun the tests with the new testing in place on master.

composer run phpunit -- --group microformats/tests/mf1

With this PR merged (like before), we are now able to pass the hcalendar/combining test.

But also like before, this still does not fix hcalendar/attendees:

Mf2\Parser\Test\MicroformatsTestSuiteTest::testMf1FromTestSuite with data set "hcalendar/attendees" ('<meta charset="utf-8">\n<div c.../div>\n', '{\n    "items": [{\n        "ty...: {}\n}', 'hcalendar/attendees')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'items' => Array (
         0 => Array (
             'properties' => Array (
-                'attendee' => Array (...)
                 'location' => Array (...)
                 'name' => Array (...)
                 'start' => Array (...)
             )
             'type' => Array (...)
+            'children' => Array (...)
         )
     )
     'rel-urls' => Array ()
     'rels' => Array ()
 )

This PR does not introduce any new test breaks, so it looks good to me for merge. But we need to decide whether we want the hcalendar/attendees test to pass or not.

Would be nice to get this branch rebased on master so it is easier to run tests.

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.

Fix vevent location backcompat
4 participants