Skip to content

Fixing a number of warnings for a cleaner build#2529

Merged
nickfloyd merged 2 commits intooctokit:mainfrom
JonruAlveus:Fix_Warnings
Aug 8, 2022
Merged

Fixing a number of warnings for a cleaner build#2529
nickfloyd merged 2 commits intooctokit:mainfrom
JonruAlveus:Fix_Warnings

Conversation

@JonruAlveus
Copy link
Contributor

I tried to also work out why the net462 tests fail on binary deserialization, but I couldn't. One for another time.

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Good stuff @JonruAlveus, just a couple of text changes where the comment notation seems off!

/// </summary>
/// <param name="repositoryId">The Id of the repository</param>
/// <param name="number">The issue number</param>
/// /// <param name="reaction">The reactionid for the issue</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// /// <param name="reaction">The reactionid for the issue</param>
/// <param name="reaction">The reactionid for the issue</param>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The comment number</param>
/// /// <param name="reaction">The reactionid for the comment</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// /// <param name="reaction">The reactionid for the comment</param>
/// <param name="reaction">The reactionid for the comment</param>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

AllowSquashMerge = allowSquashMerge;
AllowMergeCommit = allowMergeCommit;
Archived = archived;
#pragma warning disable CS0618 // Type or member is obsolete
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach much better than throwing warns.. this feels like a ubiquitous language kinda thing as well; as in we need a common comment language to communicate things such as obsolete fields, etc... so that we can set up automation or simply search when we decided to clean things up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely. Obsoletes should really be there for people trying to use the codebase we produce. If we're obsoleting something, we should really change it locally so we don't use it any more, before packaging it up etc.

Yes, definitely we need some common language, it could get very messy otherwise!

@JonruAlveus
Copy link
Contributor Author

part of #2496

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

👍

@nickfloyd nickfloyd merged commit 651d981 into octokit:main Aug 8, 2022
@nickfloyd
Copy link
Contributor

release_notes: Cleaned up build warnings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants