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

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

Conversation

AdamSpeight2008
Copy link
Contributor

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
Copy link
Contributor

Thanks!

You need to clean up a little and rebase. You have a few commits which are already applied: you have this AdamSpeight2008@6a75089 and this has already made it in: eb7de9b

@tugberkugurlu
Copy link
Contributor

@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 😄

@tugberkugurlu
Copy link
Contributor

@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 😄

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
Copy link
Member

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
Copy link
Contributor Author

Nope

@AdamSpeight2008
Copy link
Contributor Author

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

@AdamSpeight2008
Copy link
Contributor Author

I finally got to work,

@AdamSpeight2008
Copy link
Contributor Author

Demo

@Bartmax
Copy link

Bartmax commented Dec 7, 2014

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

@AdamSpeight2008
Copy link
Contributor Author

I would if VB.net's had. See #17

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.

4 participants