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

Enable nullable annotations, round 2 of x #524

Merged
merged 6 commits into from
Mar 10, 2021
Merged

Enable nullable annotations, round 2 of x #524

merged 6 commits into from
Mar 10, 2021

Conversation

iamcarbon
Copy link
Contributor

Round 2

@xoofx
Copy link
Owner

xoofx commented Mar 10, 2021

I had to merge another PR in the meantime and there are a few merge conflicts, could you have a look?

@@ -59,7 +61,7 @@ public override bool Match(InlineProcessor processor, ref StringSlice slice)
var position = processor.GetSourcePosition(slice.Start, out int globalLineIndex, out int column);
var localLineIndex = globalLineIndex - processor.LineIndex;

if (tableState == null)
if (tableState is null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind these changes (personally prefer is/is not), but it doesn't make a difference for nullable analyzers right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The analyzers pick up both patterns, but is is slightly more correct as it bypasses the equality operator and prevents use on structs. I'd like to followup with another PR after this to replace the remaining == null with is null / for consistency.

@@ -60,11 +61,11 @@ protected ParserList(IEnumerable<T> parsersArg) : base(parsersArg)
var tempCharMap = new Dictionary<char, T[]>();
foreach (var parser in this)
{
if (parser.OpeningCharacters != null && parser.OpeningCharacters.Length != 0)
if (parser.OpeningCharacters is { Length: > 0 })
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should stay consistent with the type of checks. The same pattern is used above in the same method but wasn't changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I had planned to follow up to apply these changes everywhere, but didn't want this PR to become to big. I replaced the remaining instances of this pattern to address this feedback now.

@MihaZupan
Copy link
Collaborator

Thanks, I think this is great for Markdig. Even if we might end up with a ton of !s in code rn, it's a great start.

@iamcarbon
Copy link
Contributor Author

Ready for review.

We'll likely want to refactor ReferralLinksExtension to make it's list non-null to address a possible NRE, but am avoiding functional changes in this first round.

@iamcarbon
Copy link
Contributor Author

Notable, they'll likely be at least one remaining round to enable the next batch of files. After that, we should be able to enable nullable at the project level, and remove the inline #nullable enables.

@xoofx xoofx merged commit 247cd92 into xoofx:master Mar 10, 2021
@xoofx
Copy link
Owner

xoofx commented Mar 10, 2021

Thanks!

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.

3 participants