RalphTro / epcis-event-hash-generator

ALGORITHM and SOFTWARE PROTOTYPE to uniquely identify/validate the integrity of any EPCIS event through a common, syntax-agnostic approach based on hashing. Takes an EPCIS Document (formatted in either XML or JSON-LD) and returns the corresponding hash value(s).
MIT License
8 stars 4 forks source link

Type field ordering bug fix for source, destination & bizTransactionList #82

Closed dakbhavesh closed 1 year ago

dakbhavesh commented 1 year ago

Dear @RalphTro & @Echsecutor ,

Can you please review this PR? You may find the fix quite complicated so your suggestions are welcome for simplifying the code. One more thing @Echsecutor / @RalphTro Can you please help in adjusting that code style complaint? I just couldn't find where the rules are defined.

Thanks in advance for your time to review.

RalphTro commented 1 year ago

Dear @dakbhavesh ,

MANY THANKS! I just executed the file epcisDocWithShippingAndTransportingEvent (both in jsonld and xml), and the results looks good to me:

bizTransactionList
type=https://ref.gs1.org/cbv/BTT-desadv
bizTransaction=urn:epcglobal:cbv:bt:4012345000009:ASN1099
destinationList
type=https://ref.gs1.org/cbv/SDT-possessing_party
destination=https://id.gs1.org/417/4023333000000
sourceList
type=https://ref.gs1.org/cbv/SDT-possessing_party
source=https://id.gs1.org/417/4012345000009

It also looks nice in the case when there are multiple source/destinations (as in the file epcisDocWithVariousEventTypes):

destinationList
type=https://ref.gs1.org/cbv/SDT-location
destination=https://id.gs1.org/414/4012345000122
type=https://ref.gs1.org/cbv/SDT-owning_party
destination=https://id.gs1.org/417/4012345000009

So - WELL DONE from my POV!

RalphTro commented 1 year ago

As to the failing code style checks: have you an idea, @Echsecutor?

RalphTro commented 1 year ago

Dear @dakbhavesh , I just had a look at two conflicting files that were hindering a merge (just minor things). Do you mind if I force-push into this branch?

dakbhavesh commented 1 year ago

Feel free to force-push into this branch @RalphTro. Thanks!

dakbhavesh commented 1 year ago

Dear @RalphTro,

As to that failing code style issue, I can ignore the code complexity-specific rule in the .flake8 file in the source code only for the json_to_py.py so that all checks will be green. We can later review the ignored rules when we have the bandwidth to refactor the code. It will be also good to consider type-safe i.e. utilizing python data objects to reduce the code complexity immensely however that's something we can discuss separately.

If you or @Echsecutor agree, then I can quickly adjust the .flake8 file.

Let me know, Thanks, Bhavesh

Echsecutor commented 1 year ago

Hi @dakbhavesh ! We are already ignoring a couple of things ( see https://github.com/RalphTro/epcis-event-hash-generator/blob/master/.flake8 ) and I am in favour of ignoring e.g. to long lines in config files. Regarding the "to complex" warning, there is usually benefit in taking this serious and refacturing the "to complex" code.

In the concrete case, I think the complexity is a fault in my existing code and your addition is just the last tiny bit ;) Lets create an issue to refacture the method and ignore the warning for now. :+1:

Echsecutor commented 1 year ago

To complex -> backlog: https://github.com/RalphTro/epcis-event-hash-generator/issues/85

RalphTro commented 1 year ago

Thanks @dakbhavesh and @Echsecutor!

From a functional POV, I can confirm that this branch works fine as far as I can judge (e.g., also in the case of several bizTransactions, it ordering works as expected + bizTransactionList only appears once):

bizTransactionList
type=https://ref.gs1.org/cbv/BTT-desadv
bizTransaction=urn:epcglobal:cbv:bt:5200001000008:4711
type=https://ref.gs1.org/cbv/BTT-inv
bizTransaction=urn:epcglobal:cbv:bt:5200001000008:RE1099

The only issue left really is the code style check. (As to the last minor commits from my side - these were just conflicting files hindering the merge.) I have no issues in disregarding the warning and proceed with the merge!

Echsecutor commented 1 year ago

I recommend autopep8 -ira ./ ;)

RalphTro commented 1 year ago

Thanks a lot, @dakbhavesh and @Echsecutor!