Drasil: Remove Attributes

[szymczdm]

As per the comments on #527 we should work on removing attributes. Here's the general idea on how to deal with each:

1. Derivation: create a HasDerivation lens with getDerivation accessor. Remove from Attribute. Add derivation field to QDefinition, and I think DQD. See what else.
2. SourceRef: create HasReference lens with getReferences accessor. Remove from Attributes. Add field to QDefinition
3. ShortName: create HasShortName lens. Merge this with refAdd lens (as a second step). Remove from Attributes. Add to same places that are currently instances of Referable
4. Rationale: remove. We will re-create this later.
5. Uses: remove. We will deal with this via 'inversion'.
6. Attributes: remove!

Recommended order of implementation: Start with 5 and 4 as they're the simplest. Then 2's lens, 2 add field, 2 remove from attributes, 1 lens, 1 add field, 1, remove from attributes, 3 lens, 3 add instances, 3 remove refAdd and replace with shortname, 3 remove from attributes, 6.

[samm82]

@szymczdm @JacquesCarette If the end goal is to remove Attributes, and QDefinition is getting them from QuantityDict, shouldn't we add the Derivation and SourceRef fields to QuantityDict instead? Or are we passing the Attributes from QuantityDict to QDefinition and then lensing the Derivation and References?

[JacquesCarette]

It doesn't make sense to attach a Derivation to a Quantity, as that represents just that, a quantity. However, it can make sense to have a Derivation on a QDefinition.

SourceRef is actually trickier. I think we'll have to see how they arise to figure out where to put them.

[samm82]

@JacquesCarette So then how should we go about implementing SourceRef - just wait for now?

[szymczdm]

SourceRef should show up on certain chunks by design (TMs, GDs, DDs, and IMs). I think we should look at the manually produced documents and see where Source comes up and what it contains. We might also want to see where external sources are referenced, to see if they're attached to a particular piece of knowledge or not.

I think that'll give us a better idea of what we're dealing with.

[samm82]

First, does "manually produced" refer to stable or to caseStudies?

Secondly, are these links what you mean by Source (from glassBR)?

Or are these:

[samm82]

Also, @szymczdm, are the FIXMEs an error in the stable?

[niazim3]

@JacquesCarette

For ShortName, where the instructions state to merge the HasShortName lens with refAdd, is the reasoning for this because one will be removed eventually?
Also, since refAdd :: s -> String, does that meant that the accessor for HasShortName should not eventually produce some data type ShortName (i.e. once ShortName will be removed from Attributes, the short name will just be a string that is passed and produced by functions)?

[szymczdm]

@samm82 "manually produced" refers to caseStudies (it's our reference material essentially, since the source fields were not filled in for our current "stable" versions).

As for the links, not necessarily. If you look at the DD, IM, TM, and GD in the caseStudies, you'll see some of them have "Source" fields (after Label, Equation, etc.).

Finally, the FIXMEs are exactly that -- things that need to be fixed. They aren't considered errors in stable as it stands, because they're known issues, but they should definitely be fixed if possible (preferably as another issue and on another branch).

[JacquesCarette]

@niazim3 Yes, right now the refName field acts as a shortname. So we don't want 2 "short name" concepts at the same time, as they are likely to be inconsistent and a source of errors. So we unify those into a single concept. I prefer to rename refName to shortname.

[szymczdm]

@JacquesCarette Just to chime in on that, the way referencing was implemented, refAdd is a plaintext referencing address (\label in TeX), whereas ShortName was more akin to refName (a short name used for referencing within a document that can include symbols and whatnot if required). So refAdd is only visible in the source (tex/html), whereas refName is visible in the typeset documents (pdf).

[niazim3]

@szymczdm does that mean that shortname should be merged with refName instead of refAdd?

[szymczdm]

@niazim3 Yes. If there's a particular reason @JacquesCarette had for suggesting the other change, I'm sure he'll weigh in and correct me (which means you might undo and redo some work, sorry!), but I think it was just a case of us using the wrong name.

[JacquesCarette]

Oops, sorry! I had these two things mixed up.

[niazim3]

Update

Steps remaining to close this issue:

• [ ] implement Contents' HasShortName instance definition (commented out code from Document.hs)
• [x] implement Citation's HasShortName instance definition
• [x] implement QDefinition's HasShortName instance definition
• [x] implement RelationConcept's HasShortName instance definition
• [x] #549
• [X] complete steps for 3 : ShortName (majority complete, awaiting ^)
• [x] remove D Derivation from Attribute.hs
• [ ] remove Attribute, Attributes, and HasAttributes from all files

[niazim3]

@JacquesCarette @szymczdm
@elwazana and I have noticed that there may be a conflict. He's been replacing the acro functions using the builtin RefHelpers in DocumentLanguage, which use customRef, which uses RefName.
When I was updating ShortName and created a shortname lens to merge with refName, refName was removed (after it had its type changed in PR #551).
This isn't a problem now, since we're on different branches, but this is definitely a design issue that will result in multiple conflicts when trying to merge.

[JacquesCarette]

Yes, that is a problem, and it will lead to conflicts.

The 'best' solution will be for @elwazana to merge into his branch first, as I believe that your changes for this issue are far enough along that they are all in to master. To make his life easier, the smaller the PRs you make onto master, the easier it will be for him to merge each into his branch. @elwazana you should merge master into your branch several times a day. This will make things easier overall.

[elwazana]

@JacquesCarette So should I start doing that now? or should I wait until RemoveAcroDD is merged because it's already ready to be merged?

[niazim3]

@JacquesCarette I also think anyplace I removed refAdd as a field and replaced it with shortNm (as mentioned in commit comments 0e6a51f3) I should probably bring it back, as it is likely useful to have both (refAdd for the link address (without symbols) and shortname for the link text (which may want symbols)).

[niazim3]

@szymczdm @JacquesCarette

As of commit 6afcba5, an sn (similar to the original refname) was added so that uncommenting code from Document.hs does not lead to errors immediately upon running make.

The error that's popping up now, though is:

cd build/SWHS && stack exec swhs
swhs.EXE: Language\Drasil\Document.hs:81:10-31: No instance nor default method for class operation shortname

, which makes sense since the sn is not the exact same as shortname, which causes an issue to arise when ^. shortname is called from anywhere in the code

(

Language/Drasil/Chunk/Attribute.hs:36:getShortName c = unwrap $ c ^. shortname
Language/Drasil/Reference.hs:255:makeRef r = customRef r (r ^. shortname)
Example/Drasil/DocumentLanguage/Definitions.hs:45:tmodel fs m t = Defnt TM (foldr (mkTMField t m) [] fs) (refAdd t) (t ^. shortname)
Example/Drasil/DocumentLanguage/Definitions.hs:50:ddefn fs m d = Defnt DD (foldr (mkQField d m) [] fs) (refAdd d) (d ^. shortname)
Example/Drasil/DocumentLanguage/Definitions.hs:56:gdefn fs m g = Defnt General (foldr (mkGDField g m) [] fs) (refAdd g) (g ^. shortname)
Example/Drasil/DocumentLanguage/Definitions.hs:61:instanceModel fs m i = Defnt Instance (foldr (mkIMField i m) [] fs) (refAdd i) (i ^. shortname)
Example/Drasil/GlassBR/Body.hs:453:assumpList ac = Assumption ac (ac ^. shortname)
Data/Drasil/Utils.hs:189:mappingShortNames t m = refFromType t m (m ^. shortname)

). From our discussion, it seems that this error may be fixed if Contents becomes a record type that we can lens shortname from, however, doing so would mean creating data types for all the types that produce Contents, which will affect a lot of files
https://github.com/JacquesCarette/Drasil/blob/6afcba523b59b198a501a6594bd9e01356a24c9a/code/Language/Drasil/Document.hs#L44-L65.

Is there something important that we're missing to solve this issue in a cleaner way?

[niazim3]

After speaking with @szymczdm, it seems that there are a few courses of action that can be implemented here:

1. Not ideal: update all types that are of instances of HasShortName to have shortname _ = undefined or sn _ = undefined so that code doesn't break; this is not ideal since eventually we want lenses to be used and will be removed; additionally, doing so will cause discrepancies since some functions will be using sn and others shortname, causing definitions of new functions to become more confusing (i.e. knowing which one to use in different situations)
2. update definition of makeref to use sn and the Contents data type to hold records like so
   https://github.com/JacquesCarette/Drasil/blob/86b97ce271d20c7a84e320b5f53b065eabd46dc1/code/Language/Drasil/Document.hs#L44-L95
   and lens using shortname from there (attempted; unsure of syntax (see below))
3. altogether, remove the shortname lens and use sn like refname was used before; @JacquesCarette are there negative consequences that should be considered here?

After attempting to implement course-of-action 2:

instance HasShortName  Contents where
  shortname = sn

resulted in

code\Language\Drasil\Document.hs:118:15: error:
    • Variable not in scope:
        sn :: (ShortNm -> f ShortNm) -> Contents -> f Contents
    • Perhaps you meant one of these:
        ‘sin’ (imported from Prelude), ‘snd’ (imported from Prelude),
        ‘sc’ (line 154)
      Perhaps you want to add ‘sn’ to the import list in the import of
      ‘Language.Drasil.Classes’ (Language\Drasil\Document.hs:14:1-69).

---

instance HasShortName  Table where
  shortname = sn

results in

code\Language\Drasil\Document.hs:112:24: error:
    Not in scope: type constructor or class ‘Table’
    A data constructor of that name is in scope; did you mean DataKinds?

---

instance HasShortName  Contents where
  shortname (Table _ _ _ _ _ _)     = sn

resulted in

code\Language\Drasil\Document.hs:118:14: error:
    • Couldn't match expected type ‘ShortNm -> f ShortNm’
                  with actual type ‘Contents’
    • In the pattern: Table _ _ _ _ _ _
      In an equation for ‘shortname’: shortname (Table _ _ _ _ _ _) = sn
      In the instance declaration for ‘HasShortName Contents’
    • Relevant bindings include
        shortname :: (ShortNm -> f ShortNm) -> Contents -> f Contents
          (bound at Language\Drasil\Document.hs:118:3)

\code\Language\Drasil\Document.hs:118:39: error:
    • Variable not in scope: sn :: Contents -> f Contents
    • Perhaps you meant one of these:
        ‘sin’ (imported from Prelude), ‘snd’ (imported from Prelude),
        ‘sc’ (line 152)
      Perhaps you want to add ‘sn’ to the import list in the import of
      ‘Language.Drasil.Classes’ (Language\Drasil\Document.hs:14:1-69).

---

@szymczdm @JacquesCarette Can you please offer some guidance on what the syntax should be when attempting to lens a field for a data type that has multiple constructors?
Or would it be better to follow option 3? That is, is lensing really beneficial here?

[JacquesCarette]

@szymczdm and I hashed out a design for shortnames (aka labels aka refnames). Both @niazim3 and @elwazana have independently run into issues related to these, which warrant a single, comprehensive design.

[niazim3]

Would it be possible to recieve clarification on where the shortname step (i.e. "3. ShortName: create HasShortName lens. Merge this with refAddrefName lens (as a second step). Remove from Attributes. Add to same places that are currently instances of Referable") stands as of yesterday's meeting?

There are a couple courses of action I can see following through with:

• merge #563, and wait on ContentChunk implementation before creating a lens getShortName to get the lens from each chunk that originally had a refName (replaced with name shortname in #563)
• remove attributes so we can close this issue, and track this implementation upgrade for shortname in a separate issue

[JacquesCarette]

I do believe that the two steps (create HasShortName lens, merge with refName) are still valid. But we have discovered that ContentChunk needs to be changed first to have a shortname.

The problem I see with removing attributes as a stand-alone step is that shortname used to be stored as an attribute. And I don't know if that would break things. Basically it depends on whether any code at all ever creates a chunk with attributes. If there are none, then yes, go ahead and do that step. And then this issue can be closed, as the rest is indeed captured in #563 and #567.

[niazim3]

Removing Attribute will break things because constructors link getShortName depend on ShortName String to get the short name of a chunk. However, if that is created as its own type (on a new branch), I don't think there should be any problem with going ahead to remove Attributes, other than the fact that it will be a lot of undoing of work like 62f85aab, since Attributes was added to most chunks for issue #514 and as outlined in #527).

If we can do that, this issue can be closed and the next step will be to create ContentChunks so that a shortname lens can be used.

[JacquesCarette]

The undoing is inevitable. But if removing Attribute makes things fail (either at compilation time or the tests), I won't want to merge that in to master. That's my main question about this step - it doesn't feel sufficiently independent from the shortname work.

But if removing Attribute can be done in such a way that things compile and test, go ahead.

[niazim3]

Having looked further into it, changes have been made that should be alright for merging I think (since the majority of those changes was trivially replacing variable name refname) in #570.
I see your point about how they are not independent enough to go through with the removal of attributes yet though, as in order to do so, for example, the getShortName definition will need to obtain the shortname of a chunk in a manner similar to the following: https://github.com/JacquesCarette/Drasil/blob/a27291829d4f480c4ade060c51ae8dfd65881f49/code/Language/Drasil/Chunk/Attribute.hs#L32-L36, which can be done cleanly through lensing.

[JacquesCarette]

Hmm, but other than having the function getShortName being defined 'in the wrong file', I don't see that it is too deeply twinned with attributes.

[niazim3]

It turns out there was a way to remove ShortName from Attribute (see PR #572). Now, we can focus on working on removing attributes in the following branch that has both #570 and #572 merged in: removeAttributes.

[niazim3]

This issue can be closed with merge of #575.