dotnet-podcasts icon indicating copy to clipboard operation
dotnet-podcasts copied to clipboard

Cleanup

Open IEvangelist opened this issue 2 years ago • 7 comments

Cleanup:

  • Split classes into separate files
  • Fixed logging perf issues, avoided interpolated messages, and instead used format.
  • Consistent file-scoped namespaces.
  • Use GlobalUsings where it makes sense.
  • Use readonly record struct instead of record (or record class) where possible.
  • Delete redundant CODE_OF_CONDUCT(2).md file.
  • Make classes sealed where applicable.
  • Use pattern matching where it makes sense for readability and succinctness.
  • Consistent naming of request objects.
  • Use HTTPS alternative URLs for RSS feeds when seeding DB data, where possible.
  • A bit more CSS for the recently added playback rate control.

IEvangelist avatar May 11 '22 19:05 IEvangelist

@IEvangelist could you do this pr into release/next-steps branch instead of main branch. @jamesmontemagno what do you think?

migueBarrera avatar May 11 '22 20:05 migueBarrera

@migueBarrera switched but lots of conflicts. Any reason not to just merge the next-steps stuff into main? nothing really breaking correct?

jamesmontemagno avatar May 12 '22 14:05 jamesmontemagno

next-steps has a lot of changes from the last few weeks.. @IEvangelist made his changes in main instead of next-steps. The other PR from daniel it was me who resolved the conflicts with next-steps. I can do it with this PR again, I know all the changes. Also, I can merge next-steps over main and we avoid these problems again.

migueBarrera avatar May 12 '22 19:05 migueBarrera

the next-steps branch has been merged into main. @IEvangelist you need resolve conflicts. I can't do it, it's in a repo of yours.

migueBarrera avatar May 13 '22 12:05 migueBarrera

the next-steps branch has been merged into main. @IEvangelist you need resolve conflicts. I can't do it, it's in a repo of yours.

Done...

IEvangelist avatar May 13 '22 15:05 IEvangelist

Hey @jamesmontemagno - are we good to merge this one?

IEvangelist avatar Jul 12 '22 14:07 IEvangelist

@IEvangelist just changed to merge to main, can you re-validate stuff for me.

jamesmontemagno avatar Jul 12 '22 16:07 jamesmontemagno

@IEvangelist just changed to merge to main, can you re-validate stuff for me.

Hey @jamesmontemagno - done

IEvangelist avatar Nov 07 '22 16:11 IEvangelist

@jamesmontemagno I'm curious, why was this closed? I've been doing everything that you've asked of me on this one. :octocat:

I've already contributed several features to this app and would love to know what happened here.

This PR adds a ton of value and addressed a lot of inconsistencies with how some of this stack is now documented. ☹️ Could you please let me know what needs to be improved upon, or changed?

IEvangelist avatar Dec 22 '22 04:12 IEvangelist

Would someone please progress this PR? Thanks.

lockhartsoftware avatar Dec 31 '22 12:12 lockhartsoftware