Removing asVC' hack with replacement QuantityDicts #2952

peter-michalski · 2022-03-10T21:54:42Z

For issue #1166

balacij · 2022-03-11T03:40:56Z

code/drasil-example/glassbr/lib/Drasil/GlassBR/ModuleDefs.hs

@@ -185,7 +189,7 @@ interpY = funcDef "interpY"
  ]

 interpZ :: Func
-interpZ = funcDef "interpZ" 
+interpZ = funcDef (showUID U.interpZ)


To continue the discussion we had on Teams here a bit, showUID (something that shouldn't really exist outside of ChunkDB debugging code) usage here is only desired because we want it to rely on (& align with) the information from the newly created QuantityDicts (I guess a more appropriate way to get the function name would be a "showSymbol"-like function for the implementation-stage name of the interpZ QuantityDict), but I think this would be best "cleaned up" if/when we place Funcs inside of the ChunkDBs. As they are right now, we aren't able to place them in the ChunkDBs because (a) they don't have UIDs, and (b) the ChunkDBs would require a bit more effort for us to be able to place Funcs inside of ChunkDBs so that we can still link the QuantityDict and the Func (maybe #2873).

Using a hacked up showSymbol-like function would be quite a bit superior to using showUID. It would at least be pulling from the 'right' bucket of information.

I agree, that makes sense.

Regarding the type of the symbol, should it really be a String? Specifically, is there any worth in changing it to the "Implementation-level Symbol"?

If we choose String, I think we have ~2 options:

figure out a way to convert the "Implementation-level Symbol" into a Doc/Text (this should already exist somewhere I believe), and then into extract it into a String, or

roll our own "Symbol -> String" function (of course, this creates potential room for misalignment with the existing Symbol -> Doc/Text functions)

No, it doesn't have to be String. Thinking about it, it should indeed be implementation-level symbol. Well spotted!

Thank you. That sounds good. In that case, I think it would be out of scope for this PR; continuing on #2959.

JacquesCarette · 2022-03-23T20:49:46Z

code/drasil-example/glassbr/lib/Drasil/GlassBR/ModuleDefs.hs

@@ -159,7 +163,7 @@ extractColumnCT = funcDef "extractColumn" "Extracts a column from a 2D matrix"
  ]

 interpY :: Func
-interpY = funcDef "interpY" 
+interpY = funcDef (showUID U.interpY)


Surely there's another field than UID in U.interpY which also have value "interpY" that could be used? Of this whole change, this is the one bit that sticks out as a hack, all the rest seems great.

@JacquesCarette @balacij I have made the requested changes in 17d3425.

You have - but the changes are too "brute force". You most definitely should not be importing HughesPJ into any example! The right thing to do is to write an appropriate function (that does what you just coded up) somewhere in an appropriate Drasil package, and then call it here. Figuring out the right signature is going to be a fun design problem.

@JacquesCarette would a showSymb-like function be helpful in Drasil? (Placed into Drasil/Symbol/Helpers.hs)

showSymb :: Symbol -> String showSymb a = render $ symbolDoc a

This would allow for the removal of HughesPJ from the example

interpZ :: Func interpZ = funcDef (showSymb $ symbol U.interpZ Implementation)

Yes, such a function would be good. Probably belongs in drasil-printers. I'd be tempted to put it under the ASCII printer.

@JacquesCarette I had just edited the above comment to suggest placing it in Drasil/Symbol/Helpers.hs, but I will look into placing it into drasil-printers. The latter does sound more appropriate.

@JacquesCarette The requested change to add a new function has been applied in the latest commit. The PR is updated.

JacquesCarette · 2022-03-23T20:51:10Z

code/drasil-example/glassbr/lib/Drasil/GlassBR/ModuleDefs.hs

@@ -185,7 +189,7 @@ interpY = funcDef "interpY"
  ]

 interpZ :: Func
-interpZ = funcDef "interpZ" 
+interpZ = funcDef (showUID U.interpZ)


Using a hacked up showSymbol-like function would be quite a bit superior to using showUID. It would at least be pulling from the 'right' bucket of information.

…nto 1166QDs

JacquesCarette · 2022-04-01T20:50:36Z

code/drasil-example/glassbr/lib/Drasil/GlassBR/ModuleDefs.hs

@@ -163,7 +162,7 @@ extractColumnCT = funcDef "extractColumn" "Extracts a column from a 2D matrix"
  ]

 interpY :: Func
-interpY = funcDef (showUID U.interpY)
+interpY = funcDef (showSymb $ symbol U.interpY Implementation)


These details should not be in GlassBR. The function that does

foo :: HasSymbol x => x -> String foo x = showSymb $ symbol x Implementation

should be in drasil-printers and then called from here.

@JacquesCarette The requested change is added in 243618f

JacquesCarette · 2022-04-01T20:50:46Z

code/drasil-example/glassbr/lib/Drasil/GlassBR/ModuleDefs.hs

 interpZ :: Func
-interpZ = funcDef (showUID U.interpZ)
+interpZ = funcDef (showSymb $ symbol U.interpZ Implementation)


JacquesCarette · 2022-04-01T20:51:22Z

code/drasil-printers/lib/Language/Drasil/Plain/Print.hs

@@ -180,3 +180,7 @@ fenceDocR Paren = text ")"
 fenceDocR Curly = text "}"
 fenceDocR Norm = text "\\|"
 fenceDocR Abs = text "|"
+
+-- | Helper for printing Symbols
+showSymb :: Symbol -> String


good start, but this only does 1/2 the job

peter-michalski added 5 commits March 2, 2022 14:15

create replacement QuantityDicts for interpY and interpZ

9b159a3

removed ^. uid

64ec618

removed incorrect import

90b0597

remove asVC' and turn on genCode

adadab3

use UIDs of interpY/interpZ QuantityDicts

24459ca

peter-michalski mentioned this pull request Mar 10, 2022

Abstract Data Definitions #1166

Closed

peter-michalski changed the title ~~Removing asVC' hack with replacement QuantityDicts - Issue #1166~~ Removing asVC' hack with replacement QuantityDicts Mar 10, 2022

balacij reviewed Mar 11, 2022

View reviewed changes

JacquesCarette requested changes Mar 23, 2022

View reviewed changes

peter-michalski and others added 2 commits March 28, 2022 20:31

replaced showUID

17d3425

Merge branch 'master' into 1166QDs

5fad059

balacij mentioned this pull request Mar 29, 2022

Code Funcs should take in Symbols for their "names" instead of raw Strings #2959

Open

peter-michalski added 2 commits March 29, 2022 13:18

added showSymb function

e96c0c0

Merge branch '1166QDs' of https://github.com/peter-michalski/Drasil i…

2829ddc

…nto 1166QDs

JacquesCarette requested changes Apr 1, 2022

View reviewed changes

add showHasSymbImpl func

243618f

JacquesCarette approved these changes Apr 18, 2022

View reviewed changes

JacquesCarette merged commit 560ab2a into JacquesCarette:master Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing asVC' hack with replacement QuantityDicts #2952

Removing asVC' hack with replacement QuantityDicts #2952

peter-michalski commented Mar 10, 2022 •

edited

Loading

balacij Mar 11, 2022 •

edited

Loading

JacquesCarette Mar 23, 2022

balacij Mar 25, 2022

JacquesCarette Mar 26, 2022

balacij Mar 29, 2022 •

edited

Loading

JacquesCarette Mar 23, 2022

peter-michalski Mar 29, 2022 •

edited

Loading

JacquesCarette Mar 29, 2022

peter-michalski Mar 29, 2022 •

edited

Loading

JacquesCarette Mar 29, 2022

peter-michalski Mar 29, 2022 •

edited

Loading

peter-michalski Mar 29, 2022

JacquesCarette Mar 23, 2022

JacquesCarette Apr 1, 2022

peter-michalski Apr 1, 2022

JacquesCarette Apr 1, 2022

JacquesCarette Apr 1, 2022

Removing asVC' hack with replacement QuantityDicts #2952

Removing asVC' hack with replacement QuantityDicts #2952

Conversation

peter-michalski commented Mar 10, 2022 • edited Loading

balacij Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balacij Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-michalski Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-michalski Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-michalski Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peter-michalski commented Mar 10, 2022 •

edited

Loading

balacij Mar 11, 2022 •

edited

Loading

balacij Mar 29, 2022 •

edited

Loading

peter-michalski Mar 29, 2022 •

edited

Loading

peter-michalski Mar 29, 2022 •

edited

Loading

peter-michalski Mar 29, 2022 •

edited

Loading