Tidy and refactor #226
Tidy and refactor #226
Conversation
| /// <returns></returns> | ||
| internal static HeartActivitiesIntraday GetHeartRateIntraday(this JsonDotNetSerializer serializer, DateTime date, string heartRateIntradayJson) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(heartRateIntradayJson)) | ||
| { | ||
| throw new ArgumentNullException("heartRateIntradayJson", "heartRateIntradayJson can not be empty, null or whitespace."); | ||
| throw new ArgumentNullException(nameof(heartRateIntradayJson), "heartRateIntradayJson can not be empty, null or whitespace."); |
There was a problem hiding this comment.
Should you add nameof() in the message as well? Or remove the name of the arg there?
| internal static HeartActivitiesTimeSeries GetHeartActivitiesTimeSeries(this JsonDotNetSerializer serializer, string heartActivitiesTimeSeries) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(heartActivitiesTimeSeries)) | ||
| { | ||
| throw new ArgumentNullException("heartActivitiesTimeSeries", "heartActivitiesTimeSeries can not be empty, null or whitespace."); | ||
| throw new ArgumentNullException(nameof(heartActivitiesTimeSeries), "heartActivitiesTimeSeries can not be empty, null or whitespace."); |
|
Done! |
|
hey @WestDiscGolf so... I'm a little hesitant to merge this kind of sweeping change in. My read is this is mostly style, using shorthands and some of the new C# 6 syntactical sugar. My concern is that if lumped in to all the work @AlexGhiondea has done we add more migration burden. We do have a lot of folks on the existing library, and then we do want to them to move along to My personal motivations is that we don't use these syntax / shorthand formats in our own codebase internally (didn't exist when we started), and selfishly would prefer to keep it consistent, unless a strong case is made otherwise. I do see a few type checks / safety checks that might be worth it, but seeing as how this was all done as a single commit, I can't really easily take those -- it's kind of all or nothing. |
|
Hi @aarondcoleman, Thanks for the feedback. I am however a little confused by what you mean by "migration burden"? I've not done anything which changes the API surface area. I have submitted the question about changing that to the team to discuss the number of constructors which would impact the API surface. The usage of newer C# syntax internally to the library I think fits with the move forward to being able to support dotnet core. The code has to work with the newer compilers due to the dotnet core support and functionally it is the same. I understand that some of it can look a bit odd at times, and yes I'm still learning some of the newer syntax myself (the expression body of properties is new to me!) so it can look odd. I'm happy to change the property expression body for the AccessToken back to a get/return (see updated PR). The rest of the changes I've submitted are pretty light touches which help with future refactoring (eg. the Happy to discuss further. |
No description provided.