7676 support new ietf

[pujak17]

No description provided.

[frieder-audriga]

You matched the namespace to the directory structure here. (I assume because your IDE recommended you to do this?)

However first of all in php namespaces don't have to match the folder structure. Folders are for strucutring files, while namespaces are for avoiding clashing class names.

in the tests/unit folder there are two separate namespaces: namespace OpenXPort\Test\ICalendar; and namespace OpenXPort\Test\VCard; if you simply change all of them to namespace OpenXPort\Tests\Unit; then there might be clashes because of structures with the same name.

It would be best to avoid such issues and keep these separate namespaces separate.

The nicest solution is probably to actually move the icalendar and vcard unit tests in separate subfolders of tests/unit, then the namespaces can match the path and are still separate.

[frieder-audriga]

When moving the files, please make the move its own separate commit (without other changes to the files), so git recognizes the files as moved (as opposed to deleted and new files)

[frieder-audriga]

Changes like these inflate the size of the PR, making it harder to review:

• There is no semantic difference between converting between vCard <-> JSContact and converting between vCard and JSContact. Specifically it's no improvement
  • And arguably this here might not be "Nexcloud specific test for converting" but rather "Tests for nextcloud specific converting", so the prior sentence might have been better actually
• It's not that this example is specifically egregious, and i'm not saying you can't change comments or improve small things that you notice that weren't in the initial scope of the task
• It's just that this is a very large PR (5k lines added 6k lines removed), and these small changes where I need to check if it just does the same thing in a different way (especially if it's code and not just a comment) add up
  • So if a specific change doesn't improve anything (semantics or legibility) I would appreciate if you didn't include it in the PR

[pujak17]

Yes, I will move back all the docstrings in the files wherever it does the same thing and where no special improvement has been made. Thanks for letting me know.

[frieder-audriga]

As I said it's not just the docstrings, oftentimes it's code as well, that just uses a different syntax for the same thing for no discernible reason.

I don't think it brings us much further if you now spend a lot of time backtracking everything to undo some changes.

Rather for the future, especially when it comes to code, don't just change things for the sake of changing things.

[frieder-audriga]

This comment also relates to the deletion of use Sabre\VObject\Reader; and change of

    /** @var \Sabre\VObject\Component\VCard */
    protected $vCard = null;

to

    /** @var string|null */
    protected $vCard = null;

---

You changed the semantics of the test somewhat here: Prior the test was reading the vcf file into a VCard object (from the Sabre library), but now it instead reads it into a string.

This made me concerned that perhaps you also changed the input type of the mapper/ adapter being tested.
However at least it looks like previously the VCard object was serialized back to a string again before inputting it (as a string-array) to mapToJmap. And there seem to be no other uses of the VCard object. So I guess it's fine.