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

Merge changes from rss dart fork. #32

Merged
merged 3 commits into from
Jul 16, 2023

Conversation

amugofjava
Copy link
Contributor

Hi @matthew-carroll,

This PR contains two commits which merges in the changes from the fork myself and @Feichtmeier have been working on.

The first commit brings the intl and xml dependencies up to date and migrates the XML code. The second adds support for the Podcasting 2.0 persons tag with relevant tests and improves the Dart docs for the Podcasting 2.0 classes.

@matthew-carroll
Copy link
Collaborator

@amugofjava is there a reason that all 3 of those things need to be done in a single PR? Could we instead handle those as 3 PRs, referencing 3 issues, and keep the clarity in the review and commit history?

@amugofjava
Copy link
Contributor Author

@matthew-carroll - this PR relates to the comments here #20 which I just saw as a single PR split into two commits.

I am happy to split the PR and link to issues, though if by three that's separate PRs for intl and xml that will be a little fiddler.

@matthew-carroll
Copy link
Collaborator

I'll review this all as one PR so that you don't have to rework it. In the future, I would prefer issue tickets filed for "upgrading xml", "upgrading intl", and "add Podcasting 2.0 person", and then separate PRs for each.

@@ -1,7 +1,14 @@
import 'package:xml/xml.dart';

/// Episodes that support the Podcasting 2.0 namespace can have a list of
Copy link
Collaborator

Choose a reason for hiding this comment

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

In keeping with Dart Doc practices, please add a blank line between the first sentence and whatever follows. And please adjust any other Dart Docs in this PR, accordingly.

.split(',')
.map((keyword) => keyword.trim())
.toList() ??
const <String>[],
image: RssItunesImage.parse(findElementOrNull(element, 'itunes:image')),
category: RssItunesCategory.parse(
findElementOrNull(element, 'itunes:category')),
findElementOrNull(element, 'itunes:category'),),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like there's a formatting issue here - looks like this comma should be followed by a line break, or if you don't want a line break, then there shouldn't be a comma.

Copy link
Collaborator

@matthew-carroll matthew-carroll left a comment

Choose a reason for hiding this comment

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

LGTM

@matthew-carroll matthew-carroll merged commit 8d1b769 into Flutter-Bounty-Hunters:main Jul 16, 2023
1 check passed
@TheRedSpy15
Copy link

what's the ETA on pub.dev getting this update?

@matthew-carroll
Copy link
Collaborator

I just released v3.0.1

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.

None yet

3 participants