Open-Agriculture / AgIsoStack-plus-plus

AgIsoStack++ is the completely free open-source C++ ISOBUS library for everyone
https://agisostack.com/
MIT License
187 stars 41 forks source link

changes standard version to VT version 6 #375

Closed dafiliks closed 10 months ago

dafiliks commented 10 months ago

should resolve issue #374 if I understood correctly.

GwnDaan commented 10 months ago

Also the version passed into the function is saved as a class member of the VirtualTerminalClient class, it'll be unused after these changes. So could you remove it aswell?

dafiliks commented 10 months ago

I'll try to do all these, thank you!

dafiliks commented 10 months ago

I have made another commit, check it out and let me know any feedback. Thanks.

GwnDaan commented 10 months ago

The build still seems to fail because where this function is used, the parameter "version" is still passed in: https://github.com/davidfiliks/AgIsoStack-plus-plus/blob/9f5e841d470eb57f8713a1e24accf06ecd5227f2/isobus/src/isobus_virtual_terminal_client.cpp#L1381

Furthermore, I was referring to this class member being unused after we make the changes in this PR: https://github.com/davidfiliks/AgIsoStack-plus-plus/blob/9f5e841d470eb57f8713a1e24accf06ecd5227f2/isobus/include/isobus/isobus/isobus_virtual_terminal_client.hpp#L1341 Would you be able to remove it, together with all places it is accessed/modified? That should only be inside the three set_object_pool functions.

I enabled the Github Actions checks to automatically start, try to get all checks to succeed. Again thanks for taking your time to make a contribution, highly appreciated :)

dafiliks commented 10 months ago

Thank you for helping like this, it is my first ever contribution on github. Check it out and give any feedback. Thanks.

ad3154 commented 10 months ago

Thank you for helping like this, it is my first ever contribution on github. Check it out and give any feedback. Thanks.

Well, it's cool that your first contribution is here with us! Welcome!

It's looking pretty good to me, my only comment would be that it would be nice to squash or fixup all your commits into one commit with a nice tile, something like your first one "Change VT Client version to VT version 6" or something similar.

GwnDaan commented 10 months ago

It's looking pretty good to me, my only comment would be that it would be nice to squash or fixup all your commits into one commit with a nice tile, something like your first one "Change VT Client version to VT version 6" or something similar.

@ad3154 FYI, that's also something GitHub can do for us :). It's the Squash and merge option, though it indeed is a nice habit to keep commit history clean

ad3154 commented 10 months ago

That's also something GitHub can do for us

I mean, yeah, I guess, it's just not the default on this repo so I forget it exists 😀

dafiliks commented 10 months ago

How would I merge all these commits into one?

ad3154 commented 10 months ago

How would I merge all these commits into one?

It depends on how you interact with Git. I use fork but for raw Git, it would be something like what is described here with a git rebase -i HEAD~5 followed by force pushing the result to your branch.

This is a very common thing in Git, and fairly critical part of most git workflows, so it's something to look into as you gain experience with it, but no worries if it's not something you want to deal with right now, we can just have GitHub auto-squash it.

dafiliks commented 10 months ago

fatal: It seems that there is already a rebase-merge directory, and I wonder if you are in the middle of another rebase. If that is the case, please try git rebase (--continue | --abort | --skip) If that is not the case, please rm -fr ".git/rebase-merge" and run me again. I am stopping in case you still have something valuable there.

I am not sure what to do here. This is the output after git rebase -i HEAD~5

ad3154 commented 10 months ago

I am not sure what to do here. This is the output after git rebase -i HEAD~5

This seems like the rebase exited while it was happening, like maybe it couldn't open your selected default text editor or something, or there was some conflict... Don't worry about it for now, you can cancel it with git rebase --abort. We can just squash it on here.

dafiliks commented 10 months ago

Alright, thanks.