RobThree / SimpleFeedReader

Easy to use, simple, Syndication feed reader
MIT License
32 stars 11 forks source link

Added images. Reads image elements from the feed if any. #4

Closed ghost closed 7 years ago

RobThree commented 7 years ago

Hi and thanks @keesdewit82!

Although I like your idea, I do have a few nitpicks with this PR. If you could change those I will merge this PR.

Having said that; I do appreciate the PR. The project is due for some 'refactoring' anyway; so if you can't be bothered, no hard feelings; I may change the PR myself and then merge.

ghost commented 7 years ago

Hi RobThree,

I have changed the code accordingly.

ghost commented 7 years ago

I think that AppVeyor cannot handle C# 6 language features. It errors over the collection initializer that I used: https://github.com/keesdewit82/SimpleFeedReader/blob/master/SimpleFeedReader/FeedItem.cs#L41

RobThree commented 7 years ago

It's not AppVeyor at fault here; it is/was the solution file being VS2013. I updated the solution file and, as you can see, the build now passes. I'll merge/release a new version soon. Again: thanks for the PR and implementing the changes!

RobThree commented 7 years ago

Just released 1.0.5. Thanks! 👍

theNailz commented 7 years ago

Bumping this issue; this does appear to be a problem in my VS (2015) project:

Property or indexer 'FeedItem.Images' cannot be assigned to -- it is read only

RobThree commented 7 years ago

Can you please provide a testcase to reproduce the problem?

ghost commented 7 years ago

It has no setter because collection properties should not have setters.

theNailz commented 7 years ago

Alright, my testcase: https://gist.github.com/theNailz/5386edf719dfd568af6e60df17b15daf

Copied the default normalizer source into my own class, and use the normalizer in my feedreader. Error on line 65

RobThree commented 7 years ago

Makes sense. From the hip.... I guess making it protected instead of internal should probably solve it?

theNailz commented 7 years ago

Yep, that way we can pass it in through the constructor, and set it from there.

ghost commented 7 years ago

Protected would make sense if you use FeedItem as base class. FeedItem is not sealed so you should be allowed to set the Images property from the inheriting class, thus making protected the right access modifier in this case.

RobThree commented 7 years ago

So we all agree on making it protected? I'm also fine making it public; the rest of the properties have public setters too so I don't think it's that big of a problem.

ghost commented 7 years ago

I would make it protected. It also depends on what guidelines you are used to work with. This is one of them https://msdn.microsoft.com/en-us/library/ms182327.aspx

ghost commented 7 years ago

Also @theNailz Small advice: You should not be passing stateful objects through the constructor.

RobThree commented 7 years ago

Protected won't suffice since even the DefaultFeedItemNormalizer won't be able to create a FeedItem.

I'm going for public; the FeedItem is nothing more than a DTO (or POCO if you will) anyway.

RobThree commented 7 years ago

See commit a063de3a41f7eea4992efb090c6313966d285e10 and NuGet

ghost commented 7 years ago

No problem. Making it public will work. In my opinion the right way is to use the AddRange method in the DefaultFeedItemNormalizer class to add the images. Its up to you Rob ;)

RobThree commented 7 years ago

AddRange implies an IList implementation, we have an IEnumerable which may not even implement an AddRange 😉

ghost commented 7 years ago

Whoops you got me there :)