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

Remove duplicated code #525

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Conversation

martonmiklos
Copy link
Contributor

Describe your changes

Removed duplicate codes to support Open-Agriculture/AgIsoVirtualTerminal#55

How has this been tested?

Loaded Kverneland implements in AgIsoVT

@martonmiklos martonmiklos marked this pull request as draft January 6, 2025 14:52
@martonmiklos martonmiklos force-pushed the dry_code branch 4 times, most recently from 6ae0eec to d66585e Compare January 6, 2025 17:08
@martonmiklos
Copy link
Contributor Author

@GwnDaan I do not have too much idea why the doxygen pipeline is failing, do you have any suggestions?

@GwnDaan
Copy link
Member

GwnDaan commented Jan 7, 2025

image
@martonmiklos This my way of finding what's wrong :)

@martonmiklos
Copy link
Contributor Author

image @martonmiklos This my way of finding what's wrong :)

Dauh, Ctrl+F and dynamic page load are my new enemy :D

@martonmiklos martonmiklos force-pushed the dry_code branch 3 times, most recently from 3a55997 to 49077a0 Compare January 7, 2025 16:26
@martonmiklos martonmiklos marked this pull request as ready for review January 7, 2025 16:30
@GwnDaan GwnDaan requested a review from Copilot January 7, 2025 18:22

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • isobus/src/isobus_virtual_terminal_objects.cpp: Language not supported
Copy link
Member

@GwnDaan GwnDaan left a comment

Choose a reason for hiding this comment

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

Good job! I like how you were able to reduce the duplication with a few simple base classes

@GwnDaan
Copy link
Member

GwnDaan commented Jan 7, 2025

@martonmiklos one nitpick, can you rephrase your commit message to be a little more descriptive?

e.g. refactor(vt-objects)!: replace duplicated functions with base classes

This way, someone can scrolls through to commits to get an idea of the changes made, and whether or not they are breaking to them. https://www.conventionalcommits.org/en/v1.0.0/ is pretty nice if you ask me

@GwnDaan
Copy link
Member

GwnDaan commented Jan 7, 2025

Also @ad3154, I would like to hear your thoughts on this and whether your initial concern still applies

One reason they are not combined right now is because they inherit from isobus::OutputNumber vs isobus::InputNumber for example, which allows for convenient use of isobus::VTObject::get_object_type and other things like that. In theory I'm not opposed to combining them but there will probably be other things that need to get re-done to make that happen, or at the very least you may need to override some virtual functions in creative ways

Originally posted by @ad3154 in Open-Agriculture/AgIsoVirtualTerminal#55 (comment)

@martonmiklos
Copy link
Contributor Author

@martonmiklos one nitpick, can you rephrase your commit message to be a little more descriptive?

e.g. refactor(vt-objects)!: replace duplicated functions with base classes

This way, someone can scrolls through to commits to get an idea of the changes made, and whether or not they are breaking to them. https://www.conventionalcommits.org/en/v1.0.0/ is pretty nice if you ask me

I have fixed the commit message, I will try to keep myself to the mentioned conventions in the future.

@martonmiklos martonmiklos changed the title [WIP]Remove duplicated code Remove duplicated code Jan 7, 2025
@ad3154
Copy link
Member

ad3154 commented Jan 8, 2025

Also @ad3154, I would like to hear your thoughts on this and whether your initial concern still applies

I think this does a good job at reducing duplication, and it leaves get_object_type alone which is nice, so I'm happy.

@GwnDaan GwnDaan merged commit 576eb98 into Open-Agriculture:main Jan 8, 2025
9 checks passed
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.

3 participants