Implements syndication feeds (new feature)

[llaville]

Hello @flavioheleno,

I've discovered your package recently, and I like it !

I would like to propose with this PR to add syndication feeds reading that is missing from features list.

As this project was not updated since a long time, there are probably some parts (CI) that won't work. Will seen it soon ;-)

At least all unit tests PASSED vendor/bin/phpunit tests/ --bootstrap vendor/autoload.php

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

...........................................                       43 / 43 (100%)

Time: 00:00.090, Memory: 12.00 MB

OK (43 tests, 239 assertions)

Hope you'll like my contribution ?

[flavioheleno]

hey @llaville thank you very much for your work! I really like it!

there are a few things I'd like to discuss with you before accepting/merging it.

1. what do you think about using public readonly properties for scalar typed properties instead of a method?

so instead of:

private string $name;

public function __construct(string $name) {
  $this->name = $name;
}

public function getName(): string {
  return $this->name;
}

we'd have:

public readonly string $name;

public function __construct(string $name) {
  $this->name = $name;
}

this guarantees property immutability and reduces code complexity.

2. this is just a nitpick, but for the code examples, there are a few multi-line echo that I'd like to see properly indented, like this:

// before
echo 'Found ',
count($clist),
' categories on channel ',
$clist->getChannel(),
PHP_EOL;

// after
echo 'Found ',
  count($clist),
  ' categories on channel ',
  $clist->getChannel(),
  PHP_EOL;

[llaville]

@flavioheleno Agree with your comments. I'll fix it now !

PS: for first point the compatibility with PHP 7.4 won't be kept because https://php.watch/versions/8.0/constructor-property-promotion is available since PHP 8.0

Are you agree to raise PHP min requirement to 8.0 ?

Sorry : your first point is not about constructor promotion property but readonly properties that require at least PHP 8.1
; see https://php.watch/versions/8.1/readonly

BTW, I'm agree to apply changes if you want ? including composer raising constraint

[flavioheleno]

hey, we should def push to at least ^8.1 :-D

[llaville]

Will also fixed worlflows accordingly to :

• https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/
• https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/
• https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

[llaville]

@flavioheleno ready for review

[llaville]

@flavioheleno any news about this PR review ?

[flavioheleno]

Hey @llaville, I've left a comment on it, just waiting for your reply so we can move on with it :)

[llaville]

Hey @llaville, I've left a comment on it, just waiting for your reply so we can move on with it :)

@flavioheleno : Could you please point to me your comment, I didn't see it

[flavioheleno]

does this link work? #2 (comment)

[llaville]

does this link work? #2 (comment)

Not for me !
Please re-post your comment or summarize it if it too long

[flavioheleno]

I was expecting something like this instead:

<?php
declare(strict_types = 1);

namespace Pickling\Resource\Feed;

use SimpleXMLElement;

final class Category extends News {
  public function __construct(SimpleXMLElement $xml, public readonly string $categoryName) {
    parent::__construct($xml);
  }
}

Then you'd simply use $categoryInst->categoryName instead of $categoryInst->getCategoryName() :)

It could also be even more simple and instead of $categoryName be simply $name, as it's a property of Category class.

Does it make sense or am I missing something?

[llaville]

If you want, you could envisage to use this syntax, but in this case, you 'll have to change all other source code API in same case. E.g:

• https://github.com/pecm/pickling/blob/main/src/Resource/Package/Info.php
• https://github.com/pecm/pickling/blob/main/src/Resource/Package/Release/Info.php

and so on ... I've not listed all cases here !

[flavioheleno]

you are right, I'll take the PR as-is and then work on the API later on :)

[llaville]

@flavioheleno I suggest to publish at least one release before to do this. That will avoid users (like me) that used your package in state to have BC breaks, when your API will change.

[flavioheleno]

does this link work? #2 (comment)

Not for me ! Please re-post your comment or summarize it if it too long

Sorry, although it saved my comment, without action it did not actually send the comment.. 🤦