Drasil
Drasil copied to clipboard
JacquesCarette
Does `uid` have to be a lens?
uid being a lens means that the UID of all instances of HasUID can be both read and written to. I greped for all occurrences of uid in the code base and the only place where it is actually used to set the UID is addRelToCC, which is easy to remove.
Making uid read-only would allow some UIDs to be computed on the fly. For example, currently the UID of a data definition is the same as the UID of the underlying concept, and in the meeting we decided that we should change the UID of the data definitions (see #3518 for a bit of context). However, the type DataDefinition doesn't have a field for its own UID, its uid simply points to the uid of the underlying concept contained inside. Allowing computed UIDs would enable us to prepend something like dd: or theory: to the UID of the underlying concept in the uid of a data definition.
It's definitely an oddity. Since we manually set UIDs, making HasUIDs uid read-only won't stop anyone from doing the same overriding (if that even exists). Ideally, when someone changes something about another chunk (without a theory refinement), the UID would change to indicate that they're both different. I don't quite understand your point about making uid read-only would allow some UIDs to be computed on the fly, can you elaborate? How is it different than adding an extra UID field for DataDefinitions?
Adding a UID field to DataDefinition should also work. Computing the UID on the fly is just an alternative solution. I don't think this alone justifies making uid read-only, but I don't think uid should be a lens in the first place (why should HasUID allow the UID to be modified?), and this is an example of the kind of things we could do if uid was read-only.
HasUID exposing a writable uid is useful for when we want to build chunks that are slightly remixes of others. For example, different English definitions, etc. These areas should change both the UID and the data to make sure it's different. Since UID is meant to be 1 level 'above' the Haskell code (e.g., implicitly done well, but there is an explicit way to do it too if we didn't have to manually write the Haskell code) but isn't, there are other ways to create the same 'problematic chunks' that having the writable UID allows -- for example, record syntax overrides allows the same overwrites, and manually creating new chunks with the 'right' chunk constructors both allow the same functionality. For that reason, I think it would be better to keep it but restrict usage heavily at the code-review level.
We went a bit lens crazy, long ago. As to your fundamental question: no, we never want change that field. So changing it to a pure getter would be a good improvement. (There are likely a few more fields like that).
@balacij having a write-once (at creation time) field is vastly different than having it be changeable, which is really what 'write' in the context of lens means. Creating a new chunk by reading an old chunk's UID still works just fine with read-only UID.
Sorry, @JacquesCarette, I don't quite understand what you mean to convey. It sounds like we're mostly saying the same, except for the ending action. I see the UID field as a cached copy of the calculated version that @hrzhuang is mentioning, and the 'writability' of the UID of a chunk being linked to whether or not we want to allow chunks to be made from others with changes (I do think that UIDs should be calculated from chunks). For example, if another chunk has a writable field, and a user changes it, shouldn't the UID of that chunk too? I guess this is primarily an implementation-level detail, so it doesn't matter much unless we have a reason to prefer one style over the other.
Also, related ticket: #2911
We do seem to be talking past each other.
I see the 'writeability' as being only an issue wrt to changing a UID of an existing chunk. You never want to write to a chunk in that way. That's what having SETTER in a Lens means/implies.
The things you bring up all seem to be tied to reading UIDs from existing chunks (allowed) to create a UID for a new chunk on initial creation. We may decide we don't like doing that, but there is nothing in Haskell's type system that will prevent us from doing that. However, we can switch from Lens to Getter to prevent writing in my sense.
Ok, sounds good. Regarding the other 'writable' lenses, should we be altering UIDs when 'writes' are made?
We should look at all the times we attempt to modify an existing chunk - I'm guessing all of those are, in our current design, a mistake. In other words, our current chunks may never need anything beyond Getter.
@TRISHI-L Do you think you could look into this? You will need basic familiarity with a 'lens'. The ticket goal at this point is to make the HasUID lens read-only (right now, it is a getter and a setter).
For reference material regarding lenses, you can also use the 'lens' Haddock documentation.
@TRISHI-L Do you think you could look into this? You will need basic familiarity with a 'lens'. The ticket goal at this point is to make the
HasUIDlens read-only (right now, it is a getter and a setter).For reference material regarding lenses, you can also use the 'lens' Haddock documentation.
Sure, I can look into that.
If we switch the uid field from Lens to Getter in the HasUID, we will make the UID read-only. Given that the addRelToCC function currently uses the set function with the uid lens to modify an existing concept, here is the relevant code:
addRelToCC :: (Express e, Concept c) => c -> String -> e -> RelationConcept
addRelToCC c rID = RC (set uid (mkUid rID) (cw c)) . express
And this function is used to create a RelationConcept:
mobShr :: RelationConcept
mobShr = addRelToCC mobShrI "mobShr" mobShrRel -- genDef4Label
Since addRelToCC relies on modifying the UID and cannot be used if uid is a Getter, should I remove addRelToCC and create mobShr directly with the desired UID and other properties by using functions like makeRC ?
Short answer: yes.
We really should never set a UID, it should be read only. What addRelToCC is doing is a hack, and it's good that this change is catching it.