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.

Sign up for GitHub

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

Jump to bottom

Scm version6.x.x #130

Merged
merged 8 commits into from
Apr 7, 2019
Merged

Scm version6.x.x #130

merged 8 commits into from
Apr 7, 2019

Conversation

veon82
Copy link
Contributor

@veon82 veon82 commented Nov 7, 2017

  • New class RTCVersion
  • Fix compatibility with RTC version >= 6.0.0

@WtfJoke
Copy link
Member

WtfJoke commented Nov 24, 2017

I love that change!

Would make sense to support multiple version with that check.
Would you kindly please add some tests for it, if its somehow possible.

WtfJoke
WtfJoke reviewed Nov 24, 2017
View reviewed changes
rtcFunctions.py Outdated
v2c = version2compare.split('.')
if thisver == v2c:
return True
for n,v in enumerate(thisver):
Copy link
Member

Choose a reason for hiding this comment

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

can this part be simplified? Please change the variable n and v to something more expressive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try, now I'm going to sleep cause it's 5:00am here.
Late night programming is never good :)

Copy link
Member

Choose a reason for hiding this comment

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

yeah looks like we live in the same timezone. At 5.00am programming is not good anymore^^

@gpflaum
Copy link

gpflaum commented Dec 27, 2017 •
edited
Loading

The scm version command doesn't show the RTC version in 6.0.1 and earlier, just plugin versions. The RTC version was added to the output in 6.0.2.

@WtfJoke
Copy link
Member

WtfJoke commented Dec 27, 2017 •
edited
Loading

The scm version command doesn't show the RTC version in 6.0.1 and earlier, just plugin versions

Oh... Thank you very much @gpflaum

So I guess I cant merge this pull request, because it makes it incompatible with versions previous 6.0.1.

Do you agree @veon82 ?

EDIT:
I just saw @gpflaum other comment here #105 (comment). I like both ideas, but version set by config would be more elegant.
I would set default version 5 and on 6 you could do something else.

@veon82
Copy link
Contributor Author

veon82 commented Dec 28, 2017

I apologize, I should have looked pertinent issues.
I agree on handling the version on config, I'll update the PR once you upgrade develop branch with this change, ok?

@WtfJoke
Copy link
Member

WtfJoke commented Dec 29, 2017

I'll update the PR once you upgrade develop branch with this change, ok?

Yes, thats fine

@zevanty zevanty mentioned this pull request Feb 4, 2018
Merged
@zevanty
Copy link
Contributor

zevanty commented Feb 4, 2018

I don't mean to take any credit, but I implemented the SCM version config option in #137. The SCM v6 code was directly pulled from @veon82's code, so all credits go to him.

@WtfJoke
Copy link
Member

WtfJoke commented Apr 13, 2018

Config has been extended from #137. You can do your changes @veon82

veon82 added a commit to veon82/rtc2git that referenced this pull request Apr 13, 2018
@veon82
Merged PR rtcTo#130 with SCM config version option support introduced…
8e1ab49 
… with PR rtcTo#137
@veon82
Copy link
Contributor Author

veon82 commented Apr 13, 2018

Done.
Thanks @zevanty for your PR.

@zevanty
Copy link
Contributor

zevanty commented Apr 15, 2018

And thank you for figuring out the majority of the work needed to get v6 working :)

@WtfJoke WtfJoke mentioned this pull request Jun 13, 2018
Open
@marcaurele
Copy link

I confirm that this is working in RTC 6.0.4. It would be nice to merge this PR into master already.

@WtfJoke WtfJoke mentioned this pull request Jan 25, 2019
Merged
@WtfJoke WtfJoke merged commit dc0736c into rtcTo:develop Apr 7, 2019
@WtfJoke
Copy link
Member

WtfJoke commented Apr 7, 2019

Finally merged it. Thanks a lot @veon82.

Sorry for taking ages :S

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

@WtfJoke WtfJoke WtfJoke left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@veon82 @WtfJoke @gpflaum @zevanty @marcaurele