Developer Decision Records

[t2gran]

We have talked about adding (Architectural) Decision Records as a tool to keep track of important decisions taken in developer meetings about design and architecture. This PR contain:

• Process documentation and requeierments
• A initial Decision Record log
• Template for Decision Records
• Updated documentation

This PR replace #5360. It is created based on the the OTP main repo not Entur repo to allow other users to commit to the repo branch. All comments in the old PR apply here as well, but we have not copied them over. Read the old issue and comments first and then this to get a chronological history of events regarding this PR. Sorry, for the inconvenience.

Closes #5360

[t2gran]

Note! There are many comment in the original PR, please copy over the notes if needed.

[leonardehrenfried]

I made a few tiny stylistic changes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.73%. Comparing base (05a057b) to head (688b7bd).
Report is 27 commits behind head on dev-2.x.

Additional details and impacted files

@@            Coverage Diff             @@
##             dev-2.x    #5932   +/-   ##
==========================================
  Coverage      69.73%   69.73%           
  Complexity     17315    17315           
==========================================
  Files           1960     1960           
  Lines          74267    74267           
  Branches        7603     7603           
==========================================
  Hits           51793    51793           
+ Misses         19832    19831    -1     
- Partials        2642     2643    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leonardehrenfried]

Are you sure that a record's equals use reference equality? https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/Record.html#hashCode()

If the component is of a reference type, the component is considered equal if and only if Objects.equals(this.c(), r.c() would return true.

[t2gran]

You are right this could be changed, the record uses a shallow eq/hc. Nested fields should also hava a value based eq/hc.

[abyrd]

I interpret this the same way as @leonardehrenfried: a record's implicit equals() method behaves as if it's calling Obejcts.equals() on every component of the record, not as if it's comparing them with the == operator. When any types are in use that provide semantic equality via an equals() override, implicit record equality is in fact deep. For example, a record consisting of nested records, themselves consisting of Strings would automatically have deep semantic equality.

[t2gran]

@abyrd The doc is changed. Does your comment apply to the old or new version of it?

[abyrd]

My comment was in response to the version of the text that I saw in the "conversation" tab of the PR, which is now outdated. But the revised text now says: "The default equals() and hashCode() implementation is shallow, so all nested fields need to be records or value-objects." and I am still not sure about this characterization.

Consider a record type containing two List<String> fields. Comparing two instances of this record type using the record's implicit equals() method would behave like calling Object.equals() on each field of the record. The default equals method on Lists applies the equals method to each element, and the default equals method of Strings compares their contents rather than identity. So using nothing but default equals methods, this record has deep semantic equality. Yet it is not composed of records or value objects, it is composed entirely of references to standard library collection instances.

I'm not entirely sure the meaning of the sentence is wrong, it might just be ambiguous or incomplete and need clarification. First I think we need to clarify which "default" the text is referring to - the defaults supplied for all Objects, or the implicit methods generated for record types. Then we need to clarify the intended meaning of "shallow" and "value-objects". The methods in question may technically perform a shallow traversal, but because the methods are recursive and virtual, the method definitions of many standard library classes effectively yield a deep traversal of the entire object graph. Value-objects would seem to exclude reference types, but the widely used Collections and Maps are reference types, and even if they were nested several layers deep under a record, they would apparently yield deep equality.

[leonardehrenfried]

What does this TODO mean?

[abyrd]

I feel like the description of decision records is sufficiently precise as-is, so maybe the TODO could be removed. I will suggest a couple of minor style edits though.

[t2gran]

Thank you for your suggestions, I have applied them.

[leonardehrenfried]

I'm not entirely sure that the paragraph about records is correct but I will approve anyway.

Also, I will keep an eye on the CI if the publishing of the documentation continues to work. If it breaks I will fix it.

[leonardehrenfried]

Can you fix the conflicts?

[t2gran]

How about, just approving before I fix the merge conflicts. The conflicts are related to the moving of the auto generated docs. So, each time a PR is merged (almost) the reviews are dismissed, because of new conflicts. I would prefer that you just approve both of you (@leonardehrenfried already did) and then I resolve the conflicts (moving the files) and merge immediately after that - without new approvals.

[abyrd]

I have suggested a change. If someone applies this change, I'll then approve and we can move ahead.

[t2gran]

Thank you for the approvals - I will now resolve the conflicts and then merge this PR. I will not wait for new approvals - as agreed in the developer meetings(yesterday and a few weeks back).