Support Syndication Support Rss Optional Elements by shmao · Pull Request #25625 · dotnet/corefx

shmao · 2017-12-01T01:51:54Z

I'm prototyping supporting RSS optional elements using the <docs>element. Please let me know if you have any concerns or suggestions. I'll apply this to other optional elements after this PR.

The change enable users to read/wring <docs> via SyndicationFeed.Documentation . Here's a couple of things worth to note:

Any existing applications that read/write <docs> via SyndicationFeed.ElementExtensions would still work after the change.
If SyndicationFeed.Documentation is ever used and becomes not null, SyndicationFeedFormatter.WriteTo would write <docs> with the value of SyndicationFeed.Documentation and ignore the corresponding ElementExtension.

Ref #25608.

mconnew · 2017-12-04T21:47:17Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+            using (XmlReader reader = documentationElement.GetReader())
+            {
+                SyndicationLink documentation = new Rss20FeedFormatter().ReadAlternateLink(reader, BaseUri);
+                return documentation;


ReadAlternateLink uses a UriParser which is why you need to instantiate an instance of Rss20FeedFormatter. This is just going to use the default Uri parser so we lose the benefit of the extensibility here. We should make ReadAlternateLink be a static method which takes a Uri parser as a parameter . Then do one of two things. 1) Have the class which instantiates the SyndicationFeed class set the parser in SyndicationFeed so any custom parser can be passed to ReadAlternateLink. 2) Pass the default Uri parser to ReadAlternateLink as that's what's being used here anyway.

Sounds good. I went with option 2).

mconnew · 2017-12-04T23:59:22Z

src/System.ServiceModel.Syndication/tests/BasicScenarioTests.cs

-                xmlw.Close();
+                using (XmlWriter xmlw = XmlWriter.Create(path))
+                {
+                    Rss20FeedFormatter atomFeed = new Rss20FeedFormatter(sf);


This seems oddly written. An instance of Rss20FeedFormatter is called atomFeed when we have an atom formatter?

Good catch. The naming error was there before I changed this. Here I'm fixing the usage of the Close() method which was not called when exception happened in WriteTo.

I'll fix the names.

mconnew · 2017-12-05T00:01:38Z

src/System.ServiceModel.Syndication/tests/BasicScenarioTests.cs

+                    Rss20FeedFormatter atomFeed = new Rss20FeedFormatter(sf);
+                    atomFeed.WriteTo(xmlw);
+                }



There's no verification that we round trip all data?

Yeah, I noticed this problem. But this is not a new added test. I'm fixing the usage of Close here.

mconnew · 2017-12-05T00:03:12Z

src/System.ServiceModel.Syndication/tests/TestFeeds/rssOptionalElements.xml

+    <!--
+    <image>
+      <url>http://2.bp.blogspot.com/-NA5Jb-64eUg/URx8CSdcj_I/AAAAAAAAAUo/eCx0irI0rq0/s1600/bg_Contoso_logo3-20120824073001907469-620x349.jpg</url>
+      <title>The title is not the same to the original one</title>


This is commented out XML but this is a very real URL to a real image. Do we want to remove this or sanitize the values?

There's a bug with the original implementation of reading <image>, which read different values and failed the tests. I'm commenting out this section out for now. We will need to revisit this when we add support for <image>.

stephentoub · 2017-12-05T13:45:55Z

src/System.ServiceModel.Syndication/ref/System.ServiceModel.Syndication.netcoreapp.cs

+    public partial class SyndicationFeed
+    {
+        public SyndicationLink Documentation { get{ throw null; } set{ } }
+    }


Has this been through API review? If not, we should close the PR until it's been vetted.

This has not been approved yet. I added the NO MERGE label on the PR for now. I'd like to get feedback from others and see if it's even possible or make sense to add the new APIs before sending out API request..

mconnew · 2017-12-07T21:26:19Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/Rss20FeedFormatter.cs

+            if (Feed.GetDocumentation() != null)
+            {
+                writer.WriteElementString(Rss20Constants.DocumentationTag, Feed.GetDocumentation().Uri.ToString());
+            }


Uri can be null

mconnew · 2017-12-07T21:27:58Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

        }

+        internal SyndicationLink GetDocumentation()
+        {


This is basically a property getter. Can you change this to a read only property with a name such as InternalDocumentation.

mconnew · 2017-12-07T21:35:47Z

Just the one small issue (method -> property), otherwise LGTM.

justinvp · 2017-12-12T19:08:52Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

    // NOTE: This class implements Clone so if you add any members, please update the copy ctor
    public class SyndicationFeed : IExtensibleSyndicationObject
    {
+        private readonly static List<string> s_acceptedDays = new List<string> { "monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday" };


Nit: static readonly (static before readonly)

justinvp · 2017-12-12T19:10:02Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

            set { _title = value; }
        }

+        internal SyndicationLink InternalDocumentation


Nit: Could be simplified as:

internal SyndicationLink InternalDocumentation => _documentation;

justinvp · 2017-12-12T19:10:33Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+            }
+        }
+
+        internal int? InternalTimeToLive


Nit: Could be simplified as:

internal int? InternalTimeToLive => _timeToLive;

justinvp · 2017-12-12T19:10:53Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+            }
+        }
+
+        internal Collection<int> InternalSkipHours


Nit: Could be simplified as:

internal int? InternalTimeToLive => _skipHours;

justinvp · 2017-12-12T19:11:07Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+            }
+        }
+
+        internal Collection<string> InternalSkipDays


Nit: Could be simplified as:

internal int? InternalTimeToLive => _skipDays;

justinvp · 2017-12-12T19:20:06Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+
+                        if (hour < 0 || hour > 23)
+                        {
+                            throw new ArgumentException("The hour can't be lower than 0 or greater than 23.");


String should be added to the resources.

justinvp · 2017-12-12T19:20:46Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+            return isValidTextInput(textInput) ? textInput : null;
+        }
+
+        private bool isValidTextInput(SyndicationTextInput textInput)


isValidTextInput => IsValidTextInput and can be static.

justinvp · 2017-12-12T19:23:23Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+                    case Rss20Constants.DocumentationTag:
+                        if (InternalDocumentation != null)
+                        {
+                            return true;


These can be simplified, e.g.:

switch (localName) { case Rss20Constants.DocumentationTag: return InternalDocumentation != null; case Rss20Constants.TimeToLiveTag: return InternalTimeToLive != null; case Rss20Constants.TextInputTag: return InternalTextInput != null; // etc. }

justinvp · 2017-12-12T19:23:38Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+
+                        break;
+
+                    default:


The default case isn't necessary

justinvp · 2017-12-12T19:26:14Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+        private SyndicationLink TryReadDocumentationFromExtension(SyndicationElementExtensionCollection elementExtensions)
+        {
+            SyndicationElementExtension documentationElement = elementExtensions
+                                      .Where((e) => e.OuterName == Rss20Constants.DocumentationTag && e.OuterNamespace == Rss20Constants.Rss20Namespace)


Nit: (e) => ... can be simply e => .... Applies throughout.

JonHanna · 2017-12-12T19:34:38Z

src/System.ServiceModel.Syndication/src/System/ServiceModel/Syndication/SyndicationFeed.cs

+                    _timeToLive = TryReadTimeToLiveFromExtension(ElementExtensions);
+                }
+
+                return _timeToLive.Value;


Is InvalidOperationException if TryReadTimeToLiveFromExtension returns null appropriate?

Good catch. I changed the type of the property to int?. Thanks.

shmao · 2017-12-12T21:24:17Z

@justinvp @JonHanna Thanks for reviewing the PR. I've made changes per your suggestions.

shmao · 2017-12-13T00:54:41Z

The proposed APIs have not been reviewed yet. See https://github.com/dotnet/corefx/issues/25718.

I will be out of office and be back on Jan 2, 2018. I'm leaving the PR open for now. If the the APIs are approved, please feel free to merge the PR. If anyone feels the PR being open for too long, please feel free to close it. Thanks.

shmao · 2018-01-09T01:03:32Z

@dotnet-bot test OSX x64 Debug Build

karelz · 2018-01-27T18:17:30Z

Closing, it's replaced by PR #26612

Syndication Support Documentation Element.

e77a500

Ref #25608.

shmao self-assigned this Dec 1, 2017

shmao requested review from mconnew and zhenlan December 1, 2017 01:52

shmao added the area-System.ServiceModel.Syndication label Dec 1, 2017

shmao added this to the 2.1.0 milestone Dec 1, 2017

mconnew reviewed Dec 4, 2017

View reviewed changes

Addressed PR feedback.

e1a4fe7

mconnew reviewed Dec 4, 2017

View reviewed changes

mconnew reviewed Dec 5, 2017

View reviewed changes

Minor fixes.

090405a

stephentoub reviewed Dec 5, 2017

View reviewed changes

shmao added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 5, 2017

mconnew reviewed Dec 7, 2017

View reviewed changes

mconnew approved these changes Dec 7, 2017

View reviewed changes

shmao added 5 commits December 8, 2017 16:36

Addressed PR feedback.

1fc2a0a

Added support for more optional elements.

3b3734b

Renamed tests.

d8ec56c

Add setter for collection property.

21d2722

Changed Collection Properties.

2a21b63

justinvp reviewed Dec 12, 2017

View reviewed changes

JonHanna reviewed Dec 12, 2017

View reviewed changes

shmao changed the title ~~Syndication Support Documentation Element.~~ Support Syndication Support Rss Optional Elements Dec 12, 2017

Made changes per PR feedback.

61e81d5

karelz assigned huanwu and zhenlan Jan 18, 2018

karelz added the blocked Issue/PR is blocked on something - see comments label Jan 18, 2018

huanwu assigned mconnew and unassigned huanwu, zhenlan and shmao Jan 18, 2018

mconnew mentioned this pull request Jan 27, 2018

Support Syndication Support Rss Optional Elements #26612

Merged

karelz closed this Jan 27, 2018

Conversation

shmao commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mconnew commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmao commented Dec 12, 2017

Uh oh!

shmao commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shmao commented Jan 9, 2018

Uh oh!

karelz commented Jan 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

shmao commented Dec 13, 2017 •

edited

Loading