CaringCaribou / caringcaribou

A friendly car security exploration tool for the CAN bus
GNU General Public License v3.0
738 stars 193 forks source link

Added function for processing negative response #126

Closed n135c10r closed 1 month ago

n135c10r commented 1 month ago

I did some checks for the changes, looks ok: tests check

What do you think about the typing which i add? Ninja coder probably wont add those if in the whole module no typing is used, but i thought maybe we can do it in small steps. If something is touched it can be a bit improved in the same time?

Whats your opinion about adding NegativeResponse and Response enums? This way, we wouldn't have to refer to some magic numbers from a list but we could use their UDS bytes representation names instead.

BTW, i think tests could be improved. I can try to add some. Did you thought maybe about adding pytest to the project? I find it easier to use, especially for fixtures, mocks, etc. And maybe add some linter, e.g. ruff, its quite fast?

kasperkarlsson commented 1 month ago

Hi there!

First of all - great initiative 👍

What do you think about the typing which i add? Ninja coder probably wont add those if in the whole module no typing is used, but i thought maybe we can do it in small steps. If something is touched it can be a bit improved in the same time?

That is a GREAT idea. Now that the tool has (finally) dropped Python 2.7 support, we can start adding type hints.

Whats your opinion about adding NegativeResponse and Response enums? This way, we wouldn't have to refer to some magic numbers from a list but we could use their UDS bytes representation names instead.

I generally consider magic numbers to be a bad pattern, so I am very ready to hear more about your idea here 🙂 What are you suggesting, how would such enums look?

BTW, i think tests could be improved. I can try to add some. Did you thought maybe about adding pytest to the project? I find it easier to use, especially for fixtures, mocks, etc. And maybe add some linter, e.g. ruff, its quite fast?

This is outside of my home territory, so you might very well know better than me what would be useful to add. Feel free to add suggestions in issues!

n135c10r commented 1 month ago

Fixed 😉

kasperkarlsson commented 1 month ago

Thanks a lot, this turned out great! :muscle: