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

feature: upgrade rss behaviour #347

Merged
merged 16 commits into from
Sep 6, 2024

Conversation

elementh
Copy link
Contributor

@elementh elementh commented Sep 4, 2024

Progress:

  • Add query parameter to request content in RSS
    • With default N to BlogPostsPerPage
  • Add query parameter to request N posts
  • UI Changes
  • Export content as Markup instead of plain text

Will close #343

@elementh
Copy link
Contributor Author

elementh commented Sep 4, 2024

@linkdotnet I'm having trouble with debugging during development—neither the project nor the tests will debug, and it just silently stops. I'm using Rider on Linux. Any ideas on what I might be missing?

@linkdotnet
Copy link
Owner

I do think that is a short-coming from Rider with the current net9.0 preview.
I also experience this with Rider on MacOS arm64.

Might be worth to open a issue (it seems to be related to Blazor, because my blog is the only project I can't debug ATM).

@linkdotnet
Copy link
Owner

I also have the same issue if I just create a new xUnit project with net9.0.
Will file a report.

@elementh
Copy link
Contributor Author

elementh commented Sep 4, 2024

I just added the dropdown with both RSS feeds. It looks nice but I think you should be the one deciding on the names for both feeds.

As of right now, it looks like this:

image

How should we name the links? Also, should we remove the RSS icon on the inner links?

@linkdotnet
Copy link
Owner

I would call it "Compact" and "Full". We can refer to the readme and add a section there.
Given that we will always show all blog posts, or? It just differs whether or not the content is part of the RSS Feed or not.

@elementh
Copy link
Contributor Author

elementh commented Sep 4, 2024

Given that we will always show all blog posts, or?

Maybe I missunderstood, wasn't it supposed to be a limit (following BlogPostsPerPage or query parameter numberOfBlogPosts) when requesting full content?

@linkdotnet
Copy link
Owner

Given that we will always show all blog posts, or?

Maybe I missunderstood, wasn't it supposed to be a limit (following BlogPostsPerPage or query parameter numberOfBlogPosts) when requesting full content?

Fair point - yes. That doesn't make the wording easier though.

  1. All Posts (Summary)
  2. Most recent Posts (Full Content)

Something like that?

@elementh
Copy link
Contributor Author

elementh commented Sep 4, 2024

I like that!

What should we do with the icons inside the dropdown? Stay or go?

@linkdotnet
Copy link
Owner

I like that!

What should we do with the icons inside the dropdown? Stay or go?

I would say "Bye Bye" given that we already have an icon for the header.

@linkdotnet
Copy link
Owner

Offtopic: https://youtrack.jetbrains.com/issue/RIDER-116908/Debugging-xUnit-net9.0-tests-will-crash-the-debugger-Mac-OSX-aarch64

If you wanna weigh in

@linkdotnet
Copy link
Owner

@elementh As a heads up: I migrated all tests from FluentAssertions to Shouldly. Also I enabled nullability (that was missing until now for all tests). You might want to rebase/merge from the current master.

@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

@linkdotnet sounds great, will do.

I've been trying to do the markup thing as clean as possible, but I run into some problems with the Rss20FeedFormatter, turns out there is a bug from the pre-core days that is still haunting this formatter on which it will format Markup strings wrongly if they are the content of the SyndicationItem (check this link, and this link too).

The old "simple" workarounds don't work anymore, so I have to come up with something else. There is something that would probably work, but it involves creating the feed and then manually replacing content, and that sounds so bad hahaha

@elementh elementh force-pushed the feature/upgrade-rss-behaviour branch from 6175e66 to d004ba0 Compare September 6, 2024 10:42
@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

The force push is for the rebase, sorry.

Okay, everything should be ready and working, please have a look and let me know!

@elementh elementh marked this pull request as ready for review September 6, 2024 10:43
@linkdotnet
Copy link
Owner

The force push is for the rebase, sorry.

I do that all the time. I am "Camp Rebase" ;)

Okay, everything should be ready and working

Could you also add Rate-Limiting for the Controller? In any case - big thanks for the support. I will have a look.

@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

Also, checking the feed output with a validator (https://validator.w3.org/feed/check.cgi) tells me that the image element is unknown, and on my RSS Reader I see no images from your blog posts. Should we make it a <img> instead?

@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

Could you also add Rate-Limiting for the Controller? In any case - big thanks for the support. I will have a look.

Should we use https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit or do you prefer any third party packages?

Copy link
Owner

@linkdotnet linkdotnet left a comment

Choose a reason for hiding this comment

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

Really good job! A few changes and we are there!

@linkdotnet
Copy link
Owner

Could you also add Rate-Limiting for the Controller? In any case - big thanks for the support. I will have a look.

Should we use https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit or do you prefer any third party packages?

I would go with the ASP.NET one as you proposed. If you feel very motivated you can even use the WebApplicationFactoryTests (aka SmokeTests) and try to test that (if that is somewhat manageable)

@linkdotnet
Copy link
Owner

Also, checking the feed output with a validator (https://validator.w3.org/feed/check.cgi) tells me that the image element is unknown, and on my RSS Reader I see no images from your blog posts. Should we make it a <img> instead?

Hmmm I initially found that on Stackoverflow. Have to check that with an RSS Feed Reader. If you can confirm that "img" works - go ahead.

@linkdotnet
Copy link
Owner

On feedly it works with the current setup

when the numberOfBlogPosts is not specified in the call (RSS)
@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

Let's leave it like that then, at least for now!

@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

I would go with the ASP.NET one as you proposed. If you feel very motivated you can even use the WebApplicationFactoryTests (aka SmokeTests) and try to test that (if that is somewhat manageable)

I'll do this either this afternoon or tomorrow!

@linkdotnet
Copy link
Owner

I would go with the ASP.NET one as you proposed. If you feel very motivated you can even use the WebApplicationFactoryTests (aka SmokeTests) and try to test that (if that is somewhat manageable)

I'll do this either this afternoon or tomorrow!

We can also do this in a follow up PR if you wish

@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

Sounds great, should one of us open a new issue for the rate limiting too?

@linkdotnet
Copy link
Owner

Sounds great, should one of us open a new issue for the rate limiting too?

I would keep it open. Just split the feature into 2 sep. PR's

@elementh
Copy link
Contributor Author

elementh commented Sep 6, 2024

I would keep it open. Just split the feature into 2 sep. PR's

Even better 😄

@linkdotnet linkdotnet merged commit a4785b5 into linkdotnet:master Sep 6, 2024
1 check passed
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.

Upgrade RSS behaviour to be more friendly
2 participants