JacquesCarette / Drasil

Generate all the things (focusing on research software)
https://jacquescarette.github.io/Drasil
BSD 2-Clause "Simplified" License
143 stars 26 forks source link

`References` shouldn't have `RefInfo` #2657

Closed Ant13731 closed 3 years ago

Ant13731 commented 3 years ago

Since References are being held in the chunk database for deferred lookup by UID, all of the attached RefInfo information shouldn't be with the actual Reference itself. We have already made the Ref sentence constructor do the work of holding any needed RefInfo, similar to how Ch holds whether a term should be singular, plural, or short form. RefInfo is treated as Sentence-level display information, where the user explicitly writes in that they want RefInfo to display at a certain point in a sentence.

As of right now, any reference that enters the ChunkDB gets the RefInfo value set to None (by calling ref on anything that is Referable). So it doesn't really make sense to keep it as a part of the Reference type because it should not be used inside a Reference. Leaving it may cause unwanted behaviour, as a user might unknowingly put RefInfo into a ChunkDB, causing all appearances of that reference to have the attached RefInfo when they only wanted it in one particular spot.

In addition, this has the added benefit of getting rid of RefProg.hs, since we can put that datatype into Sentence.hs. This design is similar to how Ch works, where all the Sentence-level stuff stays with the Sentence type.

JacquesCarette commented 3 years ago

Agreed.

Ant13731 commented 3 years ago

I've started implementing some changes, but I'm running into a problem that's more focused on design: DataDefinitions, InstanceModels, TheoryModels, and GeneralDefinitions all use RefInfo within their internal References. This use of RefInfo within an actual Reference shows up in the sources section of the table when each model or definition is generated. Removing this without doing anything would erase the extra RefInfo from the SRS completely. Here is a data definition from GlassBR to show how the refInfo function is being used to make a reference with additional information.

dimLL :: DataDefinition
dimLL = dd dimLLQD [ref astm2009, refInfo campidelli $ Equation [7]] Nothing "dimlessLoad"
  [qRef, aGrtrThanB, stdVals [modElas], hRef, gtfRef]

I have a few ideas on how we could go about remedying this:

  1. add a field that holds a Sentence of references to the record type for definitions and models. This would probably have the lowest impact on the rest of drasil, but I'm not sure if we really want to encode that sort of thing directly into the type of models and definitions.
  2. Convert the current record field for references (currently [Reference]) into something that takes references and any associated reference information (like [(Reference, RefInfo)]). From there, it is an easy modification to the table generator to pass through the RefInfo. This seems kind of hacky though and the HasReference class would need to change. This would also require renaming that class, as it would no longer be holding just References but RefInfo as well.
  3. Convert the current record field for references into a list of Sentences, where each element is a reference that has been put into sentence form. This way, we can hold the RefInfo along with reference UIDs in a list. It would also involve changing the HasReference class from fetching References to fetching Sentences. I think this would be okay for the most part, but leaving it open to anything that is a Sentence may be a little too flexible.
  4. Some combination of 1 and 3, where each reference is encoded as an element of a list of Sentences. We would still keep the current referencing field, but use this new field instead for generating the definition/model tables. This seems the least hacky to me, and would probably not have that much of an impact on the rest of Drasil

I'm not sure what the best move here is. They all don't seem like great options in terms of reducing the complexity of referencing, but I don't want to leave out RefInfo entirely from models/definitions.

JacquesCarette commented 3 years ago

First: we want structured information as much as possible. Sentence is slightly better than String, but not enough. Passing a Sentence around should be a last resort when we decide that the only use we'll ever make with that information is display it - now and forever. This is not so with RefInfo, so any Sentence-based solution, is not a solution.

It seems to me that we want the "references" part in DataDefinitions (etc) to be 'decorated' references (as you show above). We most definitely shouldn't remove that information.

Ant13731 commented 3 years ago

Yea, I think I see what you mean. So would a possible solution be to create and use a new decorated reference type, and then define that as part of the Referable class for when we need just the reference information? Maybe also a part of the HasReference class as well? Something like:

data DecRef = DR {ref :: Reference
                 ri :: RefInfo}

Just for those times when we need the extra information but aren't quite ready to use sentences?

JacquesCarette commented 3 years ago

We could certainly create such a type.

The problem is to decide what Referable and HasReference really mean. A DecRef doesn't really have a reference, it is a reference. Similarly, things like sections and papers should be Referable, but references themselves should be Referable.

Ant13731 commented 3 years ago

So References themselves should not be Referable? I originally thought that Referable was just anything that could be turned into a reference (and that references could be turned into references). But if we want to make the distinction between References and things that are Referable without breaking how references work, then I think there should be a little shuffling of the class structure.

Currently, we have the HasShortName, HasRefAddress, and Referable classes. As of right now, the HasRefAddress class isn't really used. Previous to the changes I made to Reference, HasRefAddress was used for some referencing functions, but not much else. Making References an instance of the Referable class reduced the need for seemingly duplicate functions. But if we want to remove that instance, then we might want the following to happen:

I agree that DecRef shouldn't be part of the HasReference class, but maybe it could be a part of the above suggestions for classes (with all the same class instances). And then, we can provide a function that fetches the RefInfo from only a DecRef when needed.

What do you think?

JacquesCarette commented 3 years ago

I think Referable ought to be anything that can be referenced - which is slightly different. We don't want pointers to pointers...

Yes, I am quite sure there will be a need for some rejigging - both what the code uses and maybe the classes. It might be that in places it is the code that incorrectly uses the member functions, rather than the classes being off.

I think your suggestions make sense - but are phrased a little too "operationally" for me. Could you give me what the 'meaning' of each changed/new class would be?

Ant13731 commented 3 years ago

Yea, I agree. I thought about this a little bit more and I think that we might not need a new class, just that the existing HasRefAddress class isn't what I thought it was. I think I was a little off in my design, so I'll try and go through my new idea. For the meanings of each class:

In clearer terms: Drasil doesn't really use reference adresses in the form of only Strings at all. Even at the printing stage, Drasil most of the printer functions still use the LblType of a Reference. They can then use the string with its intended context. There is already a function that can unwrap a LblType to get the String form, so passing around LblTypes in Drasil should work well.

I feel like I'm still learning more about Drasil's referencing (its a lot more complicated than I thought), but on the technical side of things, here is what I propose:

I'm not sure if that last point is a good idea, but I think this might work smoothly enough. After these classes are cleared up, we can worry more about the DecRef and maybe write a function to get the RefInfo needed