ev3dev / ev3dev-lang

(deprecated) language bindings for ev3dev sensors, motors, LEDs, etc.
GNU General Public License v2.0
56 stars 39 forks source link

Support change of port-name to address in language bindings #136

Closed rhempel closed 8 years ago

rhempel commented 8 years ago

This merge got a bit messy - sorry for the confusion @WasabiFan. I have generated the Python binding at this point, but since it's a breaking change we'll need to bump the Debian package number to 0.6.0 - which will of course affect @ensonic. The release of the new ev3dev image and language bindings has to be co-ordinated.

WasabiFan commented 8 years ago

I'd like to think I am at least mostly competent at this point when it comes to git... but I can delightfully say that I have absolutely no idea what's going on here. :confused:

So... here's what I do know (mostly for the comedic effect -- serious response below :wink:):

So I'd :+1: if GitHub had voting.


But seriously, I don't think that I have the requisite qualifications to decide to merge this -- chiefly because I have no idea what it does. Are the changes simple enough that you can diff the necessary commits and re-apply them to avoid having to enter the matrix to understand it? If not, I'll just trust you when you say that it deals with port-name and hope the world doesn't end when you merge it.

rhempel commented 8 years ago

I'm not sure what's going on either - and I'm pretty sure it's all my own fault. But I can say that the end result builds and runs. It's partly because I was not keeping my clone of ev3dev-lang up to date - and my New Years resolution is to keep from messing up my git commits. Is it worth squashing this mess and sorting out the changes, or shall we just hold our noses one more time :-)

ensonic commented 8 years ago

wrt ledds, in my code I have calls like:

ev3Leds.set_color(ev3.Leds.LEFT, ev3.Leds.GREEN)

I think this should still work. I was looking at the summary of changes ("Files changed tab") which is easier to read than the individual commits.

WasabiFan commented 8 years ago

I was looking at the summary of changes ("Files changed tab") which is easier to read than the individual commits.

That tab on this PR is currently just showing all the changes that have been made over the last few months. That coupled with the fact that is is currently unmergeable makes me think that this PR doesn't do what it should.

But I can say that the end result builds and runs.

What builds and runs? Autogen? Or python?

Is it worth squashing this mess and sorting out the changes, or shall we just hold our noses one more time :-)

At the moment, there are merge conflicts that are blocking this PR. I'd say you should deal with those, then diff this branch with current develop and see exactly what has changed. If it looks good, feel free to merge it. Otherwise, you'll probably need to re-do the important bits.

dlech commented 8 years ago

In cases like this, a GUI tool like gitk or Git Extensions is very helpful. No idea how @rhempel did this, but it looks like he tried to merge without actually merging.

selection_065

It looks to me like the top two commits are really the only new ones. These should be cherrypicked on top of the develop branch of ev3dev/ev3dev-lang and all will be good.

rhempel commented 8 years ago

@dlech, you're half right - I ended up trying to resolve a merge conflict by doing it manually and made it worse. Yes, the top two commits are the ones we need in the final result. I am home for lunch now but won't be able to spend much time on it. @wasabifan - builds/runs means that autogen successfully builds the Python binding, and the resulting binding passes our minimal test suite. I have asked @ddemidov and @ensonic to test the result since they have a more complete application to test. I wish I could just do this as my main job so I could spend the time and energy that it deserves!

rhempel commented 8 years ago

I'll try again tonight to get this mess resolved

rhempel commented 8 years ago

Forget it - I'm going to resubmit a less ugly one shortly