search

DotNetAnalyzers / NullParameterCheckRefactoring

Null Parameter Check Refactoring for Reference Type Parameters
MIT License
14 stars 4 forks source link

(Proof Of Concept) Order Preservation for Parameter Guard #27

Closed AdamSpeight2008 closed 8 years ago

AdamSpeight2008 commented 9 years ago

It does do the task, but has a few bugs to find and iron out. Sometime End Of Line is preserved. Comments are removed or copied.

Seem like we need that attach the trailing trivia to the closing Parenthesis of the Exception, not as I would think the statement.

tugberkugurlu commented 9 years ago

Thanks!

You need to clean up a little and rebase. You have a few commits which are already applied: you have this https://github.com/AdamSpeight2008/NullParameterCheckRefactoring/commit/6a750890c8489e719d39f813329b309f577cb8ca and this has already made it in: https://github.com/DotNetAnalyzers/NullParameterCheckRefactoring/commit/eb7de9baea7e35a3da5578405c11c5ec6ae9b128

tugberkugurlu commented 9 years ago

@sharwell branch is not synced with the upstream dev branch as far as I can tell. That's what I mean by rebase. Is there any problem there? I'm not sure what you exactly mean by this comment :smile:

tugberkugurlu commented 9 years ago

@AdamSpeight2008 thanks for the PR again. I really appreciate that you are taking the time. If I'm offending you or @sharwell in any way, I'm really sorry because that's not my intention here.

@sharwell I don't want anybody to focus on rebase operations but I at least expect synced PRs w/o merge conflicts. This PR is out of sync, cannot be reviewed and has merge conflicts. In other words, this PR is so confusing :smile:

However, even if I did not combine these commits, your workflow rules would have caused the situation to appear anyway because #14 was merged before the VB branch was complete.

I really don't understand why. What I need is synced, w/o merge conflicts PR. this PR is unhealthy.

It seems that @sharwell understands the intention and the commits inside this PR. @sharwell: can you review and get this in if possible? If not, @AdamSpeight2008: please fix the conflicts, sync your feature branch with master and resubmit PR or force push.

sharwell commented 9 years ago

Yep, I'll take a look today.

@AdamSpeight2008 Do you mind if I force-push the result to your fork so this pull request is automatically updated?

AdamSpeight2008 commented 9 years ago

Nope

AdamSpeight2008 commented 9 years ago

Implements @bartmax improvement and my tweak to the c# implementation

AdamSpeight2008 commented 9 years ago

I finally got to work,

AdamSpeight2008 commented 9 years ago

Demo

Bartmax commented 9 years ago

shouldn't use nameof(p1) instead of the string literal ?

AdamSpeight2008 commented 9 years ago

I would if VB.net's had. See https://github.com/DotNetAnalyzers/NullParameterCheckRefactoring/issues/17