search

Openvario / variod

Daemon for autonomous e-vario
4 stars 8 forks source link

Variod testbench #11

Closed bomilkar closed 4 years ago

bomilkar commented 4 years ago

Tools to run variod in a controlled environment and on machines which have no sensor H/W. sensord_mock : supplies NMEA sentences to variod like sensord would normally do. my_netcat : implements "nc -l" on platforms which don't have that functionality.

More details in the README.txt

bomilkar commented 4 years ago

This PR has just the testbench related stuff in it. It isolated in the "testbench" directory.

linuxianer99 commented 4 years ago

@bomilkar : Same here. Please rebase the delete the revert commits.

bomilkar commented 4 years ago

@linuxianer99 is there anything I can do to make this move?

kedder commented 4 years ago

@linuxianer99, I'm pretty sure @bomilkar will learn how to squash commits eventually. Meanwhile, there is an option to "squash and merge" in github on the green merge button. It might not preserve the commit authorship, but who cares, right?

I believe this is pretty useful tool for development. I was able to run it on my laptop and even heard the vario sound. After changes to variod.c is removed from this PR, I think it is good to go!

bomilkar commented 4 years ago

Rebase was the request, not squash.

bomilkar commented 4 years ago

I'll squash all commits into one if that's what it takes to make it move on. Please advise, @linuxianer99 .

bomilkar commented 4 years ago

One more point, maybe it's not clear: I added this testbench capability as an initial commit not as a change of what existed before. On the first attempt I had it munged up with other stuff changing variod. That was a bad idea on my part, so I reverted all the variod related commits and what is left is the initial commit for testbench. If you review the changes (of all commits) the you see: it only affects files in the subdir testbench. I hope that makes it clear.

DanD222 commented 4 years ago

Hi @bomilkar - I think the ultimate aim is to review just one commit instead of 17 that there currently are, so what I would recommend is performing an interactive rebase which squashes all the relevant commits into one, and then doing a force-push to overwrite the 17 commits in your PR with your one commit. You can also chose “skip” during the interactive rebase to leave out any unrelated commits rather than reverting them.

You can then repeat this process in case you make further adjustments to the code in the PR, etc.

kedder commented 4 years ago

@bomilkar If you look at "Commits" tab in this PR, you'll see 17 commits, and most of them have nothing to do with this feature. If this PR is merged like it is now, all these commits will go to the "mainline" commit history and stay there forever, confusing anyone who wants to dig into the history. To keep histoy clean, it is considered a good practice to split a feature to small number of self-contained changes with a clear purpose. To my eye, this PR doesn't deserve more than 1 commit, since it s an initial implementation of a feature.

It is a usual practice to commit a lot while you are developing a feature, but "clean up" the history before requesting to pull into the upstream, so that your "work-in-progress" commits would not contaminate the history for everyone. As @DanD222 mentioned, the common tool for that is "git interactive rebase". You can also see how the diff looks like before pushing the branch with git diff to avoid unwanted changes to squeeze into the PR.

bomilkar commented 4 years ago

OK, understand. Since it's a new feature (not a change to existing code) I will close this PR, delete the branch and start with a new PR. In this particular case, there is no history.