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

Proposed changes for enhancement: #314

Closed
wants to merge 7 commits into from

Conversation

Wind010
Copy link

@Wind010 Wind010 commented Aug 14, 2018

Copy link
Member

@ericnewton76 ericnewton76 left a comment

Choose a reason for hiding this comment

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

I might roll back some of the CSPROJ reference changes made here...

We switched to being a NetCore library now.

@Wind010
Copy link
Author

Wind010 commented Jan 18, 2019

Sounds good to me.

@Wind010
Copy link
Author

Wind010 commented Jan 18, 2019

Should I just pull latest into my fork and resolve the conflicts?

@ericnewton76
Copy link
Member

ericnewton76 commented Jan 18, 2019

That would be easiest for me... when I'm merging, it's hard to discern your code from somebody else's

When you are merging its much easier to discern your code from somebody else's ;-)

^ ^ ^ ^
Yeah, when you first read it, its weird and then it makes sense... lol

@Wind010
Copy link
Author

Wind010 commented Jan 24, 2019

I've merged down to my fork and resolved the conflicts. Basically took everything from upstream and just kept my changes.

@ericnewton76
Copy link
Member

Looks like there was a csproj merge issue in the appveyor build

@ericnewton76 ericnewton76 changed the base branch from master to develop January 27, 2019 17:48
@Wind010
Copy link
Author

Wind010 commented Jan 29, 2019

Build fixed locally, updating tests.

@Wind010
Copy link
Author

Wind010 commented Jan 30, 2019

Please review latest changes.

@twaalewijn
Copy link

@Wind010 Excuse me for asking and perhaps I am missing something but could it be that your merge commit already accidentally ended up in the main repository?

I did a fresh fork a couple of days ago and building the develop/master branches gives me errors that would have been fixed by your latest commit.

@Wind010
Copy link
Author

Wind010 commented Feb 23, 2019

@twaalewijn Yeah, it looks like the initial PR changes were merged into the main repository:
bd67c24

I've merged merged upstream master into my fork's master and resolved the breaks. All tests are successful in the latest changes in this PR.

@Wind010
Copy link
Author

Wind010 commented Feb 23, 2019

@ericnewton76 Should I close this PR and open one where I merge from my fork master to the upstream master?

@ericnewton76
Copy link
Member

thats odd, seems like it should still pickup your changes.

Did you check into your Wind010/master branch?

@Wind010
Copy link
Author

Wind010 commented Mar 4, 2019

Sorry for the late reply. Yes, master should have the latest changes with the conflicts resolved.

https://github.com/Wind010/commandline/commits/master

The broken commit was made on the 24th, it looks like the merge of my fork to main fork was done on the 27th, but the fix/proper resolve was done on the 29th.

@ebjornset
Copy link

ebjornset commented Mar 24, 2019

@Wind010, @ericnewton76 I just forked commandline, and it seems to me that this PR is not merged in yet? At least I get compilation errors for the issues this PR seems to fix.

@moh-hassan
Copy link
Collaborator

@ebjornset
Yes, You are correct.
There is a compilation error in the commit 030eb8d on Feb 1.
Last merge need to be corrected.

@Wind010, @ericnewton76
It seems that last merge cause a compilation error and need to be fixed.
I had to stop rebasing my PR with the upstream

@Wind010
Copy link
Author

Wind010 commented Apr 8, 2019

I'll open up a fresh PR to merge from commandlineparser:develop from wind010:master.

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.

5 participants