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

Create PodcastIndex extension #31

Merged
merged 6 commits into from
Mar 16, 2021

Conversation

codedmonkey
Copy link
Contributor

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC yes
QA yes

Description

Creates an extension for the PodcastIndex namespace, a new namespace that attempts to supplement the existing iTunes podcast namespace with missing features created by @daveajones and @adamc199.

@daveajones
Copy link

Would love to see this merged.

@adamc199
Copy link

This would be a potential game changer

@weierophinney
Copy link
Member

@codedmonkey Please take a look at the Psalm errors: https://travis-ci.com/github/laminas/laminas-feed/jobs/458707070#L795

(you can reproduce them locally by running composer static-analysis)

The errors generally provide links to information detailing the general type of error, and how to correct it. You can also ask in the #contributors channel of the Laminas slack if you need assistance fixing them.

@codedmonkey codedmonkey force-pushed the podcastindex branch 2 times, most recently from 2575779 to e79ceab Compare December 11, 2020 23:48
@codedmonkey
Copy link
Contributor Author

Thanks for the heads up. Looks a lot cleaner now.

Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! 👍

I added some comments and corrections. laminas-feed supports PHP's version 7.3 and higher therefore we can use the correct return types and can omit many duplications in DocBlocks.
And old notations for internal method names do not have to be used.

mkdocs.yml Outdated Show resolved Hide resolved
src/Reader/Extension/PodcastIndex/Entry.php Outdated Show resolved Hide resolved
src/Reader/Extension/PodcastIndex/Entry.php Outdated Show resolved Hide resolved
src/Reader/Extension/PodcastIndex/Entry.php Outdated Show resolved Hide resolved
src/Reader/Extension/PodcastIndex/Entry.php Outdated Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Entry.php Outdated Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Entry.php Outdated Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Feed.php Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Renderer/Feed.php Outdated Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Renderer/Feed.php Outdated Show resolved Hide resolved
Minor improvements for components automation moved this from In progress to Review in progress Dec 14, 2020
@codedmonkey codedmonkey force-pushed the podcastindex branch 4 times, most recently from 9d16aeb to 05ac15f Compare December 21, 2020 00:51
Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

The documentation needs a better introduction for people which do not know anything about "Podcast Index".
And some more unit tests for exception are needed.

Thanks in advance! 👍

docs/book/extensions/podcast-index.md Outdated Show resolved Hide resolved
docs/book/extensions/podcast-index.md Outdated Show resolved Hide resolved
docs/book/reader.md Outdated Show resolved Hide resolved
mkdocs.yml Outdated Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Entry.php Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Entry.php Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Entry.php Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Entry.php Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Feed.php Show resolved Hide resolved
src/Writer/Extension/PodcastIndex/Feed.php Show resolved Hide resolved
@froschdesign froschdesign added this to the 2.14.0 milestone Dec 21, 2020
@codedmonkey codedmonkey force-pushed the podcastindex branch 2 times, most recently from 77f6d31 to f5b40c9 Compare December 29, 2020 22:29
psalm-baseline.xml Outdated Show resolved Hide resolved
psalm-baseline.xml Outdated Show resolved Hide resolved
psalm-baseline.xml Outdated Show resolved Hide resolved
@weierophinney
Copy link
Member

@codedmonkey Due to a snafu with how Travis-CI allocates OSS hours to projects, we had to put this on the backburner until we had GHA setup for doing CI. I've just rebased your branch against the current 2.14.x source, removing your commit with Psalm baseline changes. If you go to the files tab, you'll see annotations from Psalm detailing what needs to change. What's nice is that it now notes unchanged files that have new issues flagged separately, so we can ignore those during our review. 😄

There are a few related to your new functionality, however...

@codedmonkey
Copy link
Contributor Author

Thanks @weierophinney, the new system looks very slick.

I finally got my head around how to use Psalm so I regenerated the Psalm baseline for my changes.

Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

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

The Psalm errors are unrelated to this pull request and must be solved in a separate pull request or with version 3 of this component.

The title of the new documentation page needs an update and the comments for the CS fixer should use the new syntax.

Thanks in advance! 👍

@codedmonkey
Copy link
Contributor Author

codedmonkey commented Feb 18, 2021

I moved the Psalm commit into it's own branch at codedmonkey/laminas-feed#podcastindex-psalm

@froschdesign
Copy link
Member

froschdesign commented Feb 18, 2021

@codedmonkey

I moved the Psalm commit into it's own branch at codedmonkey/laminas-feed#podcastindex-psalm

I'm sorry, my previous comment was misleading. I mean the open error message on Laminas\Feed\Reader\Extension\AbstractEntry and Laminas\Feed\Writer\Extension\AbstractRenderer and so on. Everything else was right. No need to extract this!

@froschdesign
Copy link
Member

froschdesign commented Feb 18, 2021

@codedmonkey
Sorry for the trouble!

The Psalm error messages on Laminas\Feed\Reader\Extension\AbstractEntry or Laminas\Feed\Writer\Extension\AbstractRenderer were listed as not belonging to the changed files of this pull request. And if we added these problems here as well, it would become endless.

See "Unchanged files with check annotations": https://github.com/laminas/laminas-feed/pull/31/files

I hope I have explained it more clearly now.

@froschdesign
Copy link
Member

froschdesign commented Feb 18, 2021

@codedmonkey
Damn, the new syntax for the CS fixer does not work here because the used version 1 of laminas-coding-standard uses an old version of PHP_CodeSniffer. 😞

Can you change that? After that we have everything.

Signed-off-by: Tim Goudriaan <tim@codedmonkey.com>
Signed-off-by: Tim Goudriaan <tim@codedmonkey.com>
Signed-off-by: Tim Goudriaan <tim@codedmonkey.com>
Signed-off-by: Tim Goudriaan <tim@codedmonkey.com>
@ocean
Copy link
Contributor

ocean commented Mar 1, 2021

Sorry I missed this conversation - @froschdesign do you still need me to update my code to change the comments? Or has it been covered by a later merge?

Remaining errors are due to using mixed values, or using generic associative array data structure as properties, making verification impossible.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney merged commit e92fc6f into laminas:2.14.x Mar 16, 2021
Minor improvements for components automation moved this from Review in progress to Done Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

7 participants