features/AddTwitterHandle #721

saamerm · 2021-03-23T23:49:25Z

Adds twitter handle for authors that have a matching "FirstName LastName" as their feed provides
Also, all authors currently have twitter handles stored in the solution, so we don't have to worry about authors that don't have twitter handles. In the chance that the handle is not active, it will still not break anything
Please ignore the whitespace changes to the LeomarisReyes.cs file

Testing

Tested in local host to make sure the RSS feed shows it correctly and doesn't cause an exception otherwise

Live in action:

Note that this also causes the titles in the Preview to have the twitter handle as you can see:

* Removing Leomaris to fix the error on the preview page

* Adds twitter handle for authors that have a matching name as what their feed provides **Testing** ------------ * Tested in local host to make sure the RSS feed shows it correctly and doesn't cause an exception otherwise

Cheesebaron · 2021-03-27T10:35:49Z

src/Firehose.Web/Infrastructure/NewCombinedFeedSource.cs

+            // This foreach section is responsible for adding the Twitter handles
+            foreach (var item in feed.Items)
+            {
+                string newTitleText = string.Empty;
+                if (item.Authors.Count > 0)
+                    newTitleText = Tamarins.Where(x => x.FirstName + " " + x.LastName == item.Authors[0].Name).FirstOrDefault()?.TwitterHandle ?? string.Empty;
+                newTitleText = String.IsNullOrEmpty(newTitleText) ? item.Title.Text : item.Title.Text + " @" + newTitleText;
+
+                if (!string.IsNullOrWhiteSpace(item.Title.Type))
+                {
+                    string type = item.Title.Type == "text" ? "Plaintext" : item.Title.Type;
+                    TextSyndicationContentKind textKind = (TextSyndicationContentKind)
+                        Enum.Parse(typeof(TextSyndicationContentKind), type, ignoreCase: true);
+                    item.Title = new TextSyndicationContent(newTitleText, textKind);
+                }
+                else
+                {
+                    item.Title = new TextSyndicationContent(newTitleText);
+                }
+            }


Please encapsulate in a method

Encapsulated! Good point

Cheesebaron · 2021-03-27T10:50:54Z

src/Firehose.Web/Infrastructure/NewCombinedFeedSource.cs

+                string newTitleText = string.Empty;
+                if (item.Authors.Count > 0)
+                    newTitleText = Tamarins.Where(x => x.FirstName + " " + x.LastName == item.Authors[0].Name).FirstOrDefault()?.TwitterHandle ?? string.Empty;
+                newTitleText = String.IsNullOrEmpty(newTitleText) ? item.Title.Text : item.Title.Text + " @" + newTitleText;
+
+                if (!string.IsNullOrWhiteSpace(item.Title.Type))
+                {
+                    string type = item.Title.Type == "text" ? "Plaintext" : item.Title.Type;
+                    TextSyndicationContentKind textKind = (TextSyndicationContentKind)
+                        Enum.Parse(typeof(TextSyndicationContentKind), type, ignoreCase: true);
+                    item.Title = new TextSyndicationContent(newTitleText, textKind);
+                }
+                else
+                {
+                    item.Title = new TextSyndicationContent(newTitleText);
+                }


This improves readability a bit. Fitting everything into newTitleText is a bit of a smell to me.

Also, I prefer not to use ternaries if possible.

Suggested change

string newTitleText = string.Empty;

if (item.Authors.Count > 0)

newTitleText = Tamarins.Where(x => x.FirstName + " " + x.LastName == item.Authors[0].Name).FirstOrDefault()?.TwitterHandle ?? string.Empty;

newTitleText = String.IsNullOrEmpty(newTitleText) ? item.Title.Text : item.Title.Text + " @" + newTitleText;

if (!string.IsNullOrWhiteSpace(item.Title.Type))

{

string type = item.Title.Type == "text" ? "Plaintext" : item.Title.Type;

TextSyndicationContentKind textKind = (TextSyndicationContentKind)

Enum.Parse(typeof(TextSyndicationContentKind), type, ignoreCase: true);

item.Title = new TextSyndicationContent(newTitleText, textKind);

}

else

{

item.Title = new TextSyndicationContent(newTitleText);

}

var twitterHandle = string.Empty;

if (item.Authors.Count > 0)

{

twitterHandle = Tamarins.FirstOrDefault(x =>

$"{x.FirstName} {x.LastName}" == item.Authors[0].Name)?.TwitterHandle ?? string.Empty;

}

var title = item.Title.Text;

if (!string.IsNullOrEmpty(twitterHandle))

title += $"@{twitterHandle}";

if (!string.IsNullOrWhiteSpace(item.Title.Type))

{

string type = item.Title.Type == "text" ? "Plaintext" : item.Title.Type;

var textKind = (TextSyndicationContentKind)

Enum.Parse(typeof(TextSyndicationContentKind), type, ignoreCase: true);

item.Title = new TextSyndicationContent(title, textKind);

}

else

{

item.Title = new TextSyndicationContent(title);

}

Removed newTitleText and got rid of ternaries

Cheesebaron · 2021-03-27T10:53:38Z

src/Firehose.Web/Infrastructure/NewCombinedFeedSource.cs

+            {
+                string newTitleText = string.Empty;
+                if (item.Authors.Count > 0)
+                    newTitleText = Tamarins.Where(x => x.FirstName + " " + x.LastName == item.Authors[0].Name).FirstOrDefault()?.TwitterHandle ?? string.Empty;


This matching code seems to be a bit fragile and could perhaps lead to many misses depending on what the author has set up their feed to return.

Yup, @Cheesebaron, I couldn't find a different way to find a match except by comparing with item.Authors[0].Name. Do you have any other ideas?

Significantly changed this, and just ended up using the function used by the Preview page

saamerm · 2021-04-15T23:54:06Z

@Cheesebaron made the requested changes. Could you please review again?

Cheesebaron · 2021-04-16T09:43:18Z

@saamerm it doesn't build

src\Firehose.Web\Infrastructure\NewCombinedFeedSource.cs(79,61): Error CS0103: The name 'GetFilterFunction' does not exist in the current context

saamerm · 2021-04-18T16:57:02Z

@Cheesebaron forgot to commit the function. Please review now

Cheesebaron · 2021-04-20T07:45:31Z

src/Firehose.Web/Infrastructure/NewCombinedFeedSource.cs

+            {
+                var host = itemUrl.Host.ToLowerInvariant();
+
+                if (host.Contains("medium.com"))


Some pattern matching and splitting out into separate methods would do very good here

Cheesebaron · 2021-04-20T07:46:08Z

src/Firehose.Web/Infrastructure/NewCombinedFeedSource.cs

+        private bool MatchesAuthorUrls(IAmACommunityMember author, IEnumerable<Uri> urls, SyndicationItem item)
+        {
+            var authorHosts = author.FeedUris.Select(au => au.Host.ToLowerInvariant()).Concat(new[] { author.WebSite.Host.ToLowerInvariant() }).ToArray();
+            var feedBurnerAuthors = author.FeedUris.Where(au => au.Host.Contains("feeds.feedburner")).ToList();


All these fields don't seem relevant to all cases. Move closer to actual usage.

Also calling to list isn't necessary here.

Cheesebaron · 2021-04-20T07:47:25Z

src/Firehose.Web/Infrastructure/NewCombinedFeedSource.cs

+        TextSyndicationContentKind GetTextKind(string type)
+        {
+            if (type == "text")
+                return (TextSyndicationContentKind)Enum.Parse(typeof(TextSyndicationContentKind), "Plaintext", true);


Isn't it possible to return the actual Kind here instead of having to parse an enum?

Cheesebaron · 2021-04-20T07:47:51Z

src/Firehose.Web/Infrastructure/NewCombinedFeedSource.cs

+            if (type == "text")
+                return (TextSyndicationContentKind)Enum.Parse(typeof(TextSyndicationContentKind), "Plaintext", true);
+            else
+                return (TextSyndicationContentKind)Enum.Parse(typeof(TextSyndicationContentKind), type, true);


Use TryParse, not all types will match an enum

saamerm added 5 commits September 13, 2020 21:41

bugs/PreviewError-Leomaris

910aa36

* Removing Leomaris to fix the error on the preview page

features/AddTwitterHandle

0a3f8ad

* Adds twitter handle for authors that have a matching name as what their feed provides **Testing** ------------ * Tested in local host to make sure the RSS feed shows it correctly and doesn't cause an exception otherwise

Merge branch 'master' of https://github.com/saamerm/planetxamarin

664f99f

Update Firehose.Web.csproj

bd83ac1

Create LeomarisReyes.cs

f76da35

saamerm mentioned this pull request Mar 23, 2021

WIP: enhancement/670-TweetPostAuthor #674

Open

Cheesebaron suggested changes Mar 27, 2021

View reviewed changes

saamerm added 4 commits April 10, 2021 11:57

Improved the code

6fc0d41

Update NewCombinedFeedSource.cs

605ba2f

Update NewCombinedFeedSource.cs

65aa68a

Update NewCombinedFeedSource.cs

687bc4f

Committed the GetFilterFunction

d6d4000

saamerm mentioned this pull request Apr 18, 2021

Medium Feed Items don't show author name and associated picture anymore #724

Open

Cheesebaron suggested changes Apr 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

features/AddTwitterHandle #721

features/AddTwitterHandle #721

saamerm commented Mar 23, 2021 •

edited

Cheesebaron Mar 27, 2021

saamerm Apr 10, 2021

Cheesebaron Mar 27, 2021

saamerm Apr 10, 2021

Cheesebaron Mar 27, 2021

saamerm Apr 10, 2021

saamerm Apr 11, 2021

saamerm commented Apr 15, 2021

Cheesebaron commented Apr 16, 2021

saamerm commented Apr 18, 2021

Cheesebaron Apr 20, 2021

Cheesebaron Apr 20, 2021

Cheesebaron Apr 20, 2021

Cheesebaron Apr 20, 2021

features/AddTwitterHandle #721

Are you sure you want to change the base?

features/AddTwitterHandle #721

Conversation

saamerm commented Mar 23, 2021 • edited

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saamerm commented Apr 15, 2021

Cheesebaron commented Apr 16, 2021

saamerm commented Apr 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saamerm commented Mar 23, 2021 •

edited