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

Clean up several minor issues and add TODOs #458

Merged
merged 5 commits into from Jan 13, 2019

Conversation

Projects
3 participants
@EraYaN
Copy link
Member

commented Jan 6, 2019

Add Argument*Exceptions now use proper nameof operators.

Added exception messages to quite a few Argument*Exceptions.

Fixed re throwing to be proper syntax (without variable name).

Added a ton of null checks. (This is only a start, there are about 500 places that need proper null handling)

Added some TODOs to log certain exceptions.

@EraYaN EraYaN changed the title Mayor code cleanup WIP: Mayor code cleanup Jan 6, 2019

@Bond-009

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Looks good, but I would add braces around the ones that don't have any.

@EraYaN

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2019

I didn't want to reformat the document past the very basic functional fixes. That would make this an even harder to read pull request.

Should I still?

@EraYaN EraYaN changed the title WIP: Mayor code cleanup WIP: Major code cleanup Jan 7, 2019

@EraYaN EraYaN changed the title WIP: Major code cleanup Major code cleanup Jan 7, 2019

@EraYaN

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Alright one can look though all static methods to find ones that should not exist. Or really should be on another class.

Also the verbs are very often wrong. Tons of Get* methods should be Convert* or if move tot the proper class As*.

Mayor code cleanup
Add Argument*Exceptions now use proper nameof operators.

Added exception messages to quite a few Argument*Exceptions.

Fixed rethorwing to be proper syntax.

Added a ton of null checkes. (This is only a start, there are about 500 places that need proper null handling)

Added some TODOs to log certain exceptions.

Fix sln again.

Fixed all AssemblyInfo's and added proper copyright (where I could find them)

We live in *current year*.

Fixed the use of braces.

Fixed a ton of properties, and made a fair amount of functions static that should be and can be static.

Made more Methods that should be static static.

You can now use static to find bad functions!

Removed unused variable. And added one more proper XML comment.
@joshuaboniface

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

Currently failing Drone checks with this error:

/usr/share/dotnet/sdk/2.2.102/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.CrossTargeting.targets(31,5): error : The 'Publish' target is not supported without specifying a target framework. The current project targets multiple frameworks, please specify the framework for the published application. [/drone/src/ThirdParty/taglib-sharp/src/taglib-sharp.csproj] 
@EraYaN

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

I know, the fix is in the build system commit. Didn’t want to put the same fix everywhere.

I can pull it in here as well, if needed.

@EraYaN

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

Alright I added a merge commit made with git merge -s recursive -X ours dev so GitHub feels it's "clean."

@joshuaboniface joshuaboniface requested review from Bond-009 and JustAMan Jan 12, 2019

@Bond-009
Copy link
Member

left a comment

I still don't like the missing braces. But that can happen in a separate PR.
Most of these comments are nitpicks, just want to keep a nice consisted style.
The indentation of some files seems messed up.

Show resolved Hide resolved DvdLib/DvdLib.csproj
Show resolved Hide resolved Emby.Dlna/ConnectionManager/ServiceActionListBuilder.cs Outdated
Show resolved Hide resolved Emby.Naming/Video/VideoListResolver.cs Outdated
Show resolved Hide resolved Emby.Server.Implementations/Devices/DeviceId.cs Outdated
Show resolved Hide resolved Emby.Server.Implementations/LiveTv/LiveTvDtoService.cs
Show resolved Hide resolved RSSDP/DeviceEventArgs.cs Outdated
Show resolved Hide resolved RSSDP/DeviceUnavailableEventArgs.cs Outdated
Show resolved Hide resolved RSSDP/HttpRequestParser.cs Outdated
Show resolved Hide resolved RSSDP/HttpResponseParser.cs Outdated
Show resolved Hide resolved RSSDP/IEnumerableExtensions.cs Outdated

@Bond-009 Bond-009 added the cleanup label Jan 12, 2019

@EraYaN EraYaN requested review from joshuaboniface and nvllsvm as code owners Jan 12, 2019

@EraYaN

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

When this get's merged, the drone build breaks. So I either pull in changes from the build PR to unbreak the drone builds or we merge that one this weekend as well.

@Bond-009
Copy link
Member

left a comment

Looks good.
Things that should to be fixed (at a later date):

  • BDInfo shouldn't depend on MediaBrowser.Model
  • Add braces for if blocks
@EraYaN

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

The braces can be added in a big reformat/refactor PR. If this can be merged this weekend I can do it tomorrow.

@joshuaboniface joshuaboniface changed the title Major code cleanup Clean up several minor issues and add TODOs Jan 13, 2019

@joshuaboniface joshuaboniface merged commit dad5ca6 into jellyfin:dev Jan 13, 2019

1 check passed

continuous-integration/drone/pr Build is passing
Details

@EraYaN EraYaN deleted the EraYaN:code-cleanup branch Jan 13, 2019

@joshuaboniface joshuaboniface referenced this pull request Jan 21, 2019

Merged

Release 10.1.0 #651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.