search

cogu / autosar

A set of python modules for working with AUTOSAR XML files
MIT License
353 stars 160 forks source link

Support for parsing Autosar2 Version? #76

Closed malneni closed 3 years ago

malneni commented 3 years ago

I am maintaining option for parsing autosar revision 2 in my Fork branch as i need it. I am wondering if it is Ok try to merge with main track.

In this branch , I have Added minimum support for Autosar version parse Added comment for DataType Added length for Signal length Added tags that are not implemented but to avoid system xml parse crash

If it's not OK, I will maintain it separate.

cogu commented 3 years ago

Hi @malneni,

I do appreciate the effort but regret to say that I see 3 issues with this pull request.

The first issue is the lack of proper unit tests. For this reason alone I will reject the pull request.

The second issue is your reuse of the AtomicSoftwareComponent class which is a base class in this Python module. The fact that the class name happens to match an existing AUTOSAR2 XML tag name is a coincidence as I had no knowledge about AUTOSAR2 during design/implementation (I just wanted to differentiate it from CompositionComponent class). Your code sort of works since Python uses duck-typing (which by itself isn't an issue).

The third reason is that there hasn't been any other requests for AUTOSAR2 support. If there was a more general demand for AUTOSAR2 I would have considered taking in the pull-request. So for now I think it would be best that you maintain your code on a separate fork.

cogu commented 3 years ago

Rejected due reasons mentioned in previous comment