CodeInfo support #57
CodeInfo support #57
Conversation
DariuszDepta
changed the title
There was a problem hiding this comment.
Functionality itself looks good to me, but let's try to keep this non-breaking. I'd suggest to keep the old signature for store_code, setting a default creator in there and adding a new function with the creator param added.
I'm fine with the cosmetic changes, but would be nice to have these as a separate PR in the future, as it makes reviewing the actual changes easier.
There was a problem hiding this comment.
Functionality is fine, I think; I am not sure about how to properly approach calculating checksum - I am aware it is what I suggested, but I know it is not perfect, and I don't have the perfect solution. More in the comment.
I have some more comments to address there.
What I don't really like are changes that are personal preferences in the external code. Example - changing submsg to sub_msg. It is a long-living code that is nothing wrong with; it is our personal preference, while submsg is far more common in the ecosystem. In general - changing names because you like some better is not a good way, as this way everyone would change half of the existing names in the project because he likes some better.
This does not apply to obvious typos or grammar mistakes in documentation - those things are okay to fix; I am thinking about changing the naming convention out of nowhere. If you think naming in the project is subject to improve, we can have a discussion about it, but arbitrarily changing naming, especially in the code not affected in other way is a terrible practice, and also - because it is noisy it is difficult to pick up by the reviewer if he would disagree.
There was a problem hiding this comment.
Overall, it is good, but I prefer something else to the test approach - just improving test coverage blindly to fix annoying tarpaulin is not a way to go. When I asked you "why coverage dropped," it didn't mean that "we cannot merge it because coverage dropped". It men's "which parts you add which is not covered?" If it is a Debug trait, I am fine. On the other hand - the target right now is ~75%, which means that for every new code you write, only 75% has to be covered (target coverage is what we already have), and you should be able to reach it without logging testing.
closes #49
This PR adds CodeInfo support as requested in issue #49.