GOOL's buildClass should use module name for class name

[bmaclach]

The GOOL function buildClass currently takes a Label parameter for the name of the class. However, this class name should always match the module/file name (because some languages, like Java, require this). Thus, the Label parameter should be removed and instead the module name can be read from the state (using getModuleName) and used as the class name.

This could be a good first issue for someone to get acquainted with GOOL.

[muhammadaliog3]

@bmaclach
The Important Code
The buildclass function

buildClass :: (RenderSym r) => Label -> Maybe Label -> [CSStateVar r] -> 
  [SMethod r] -> SClass r
buildClass n = S.intClass n public . inherit

Question 1: What is the type of "public . inherit" from the build class function, is it r ParentSpec?
The intClass function

class (BlockCommentSym r) => RenderClass r where
  intClass :: Label -> r (Scope r) -> r ParentSpec -> [CSStateVar r] 
    -> [SMethod r] -> SClass r
    
  inherit :: Maybe Label -> r ParentSpec
  implements :: [Label] -> r ParentSpec

  commentedClass :: CS (r (BlockComment r)) -> SClass r -> SClass r

We need to get rid of the "Label" parameter and instead just use the string from getModuleName

getModuleName :: FS String
getModuleName = gets (^. currModName)

Initial Solution
So the problem is essentially extracting the string from getModuleName and feeding it to the intClass function. My first real solution was to extract the string from getModuleName in a do statement and then run the intclass function inside the do statement. It looked like this

buildClass = do 
  n <- getModuleName
  S.intClass n public . inherit

Which outputs the following error
"drasil-gool > Expected type: Maybe Label
drasil-gool > -> [CSStateVar r] -> [SMethod r] -> SClass r
drasil-gool > Actual type: transformers-0.5.6.2:Control.Monad.Trans.State.Lazy.StateT
drasil-gool > GOOL.Drasil.State.FileState Data.Functor.Identity.Identity b0
n <- getModuleName"

Question 2: I do not understand why I am getting this error. Obviously "n <- getModuleName" is an intermediate step, as it is the next line the returns the desired function, but why does haskell me to output the appropriate types in the first line.

The second problem with this solution is that in Haskell at the end of a do statement you must return value wrapped in monad. So the last line should not work, because it would have type "[CSStateVar r] -> [SMethod r] -> SClass r" which is a function, not a monadic value.

[bmaclach]

Question 1:
. in Haskell is function composition. So it is not public . inherit but rather (S.intClass n public) . inherit, and its type is Maybe Label -> [CSStateVar r] -> [SMethod r] -> SClass r. buildClass is currently written in "point-free" style. The following is equivalent to the current implementation of buildClass:

buildClass n p stVars methods = S.intClass n public (inherit p) stVars methods

But, as you can see, using "point-free" style is cleaner. HLint will even suggest using point-free, where possible.

Question 2:
Your last paragraph is correct, and that is why you are getting an error. When you switch to using do notation, you can't use point-free style any more because you want the last line of your do notation to be a concrete, monadic value, not a function. So you need to explicitly pass arguments to inherit and S.intClass until you are left with a non-function value (similar to the code I wrote above for Question 1).

[muhammadaliog3]

Questions for related to Haskell

First question
Originally my attempt was

buildClass p stVars methods = do 
  n <- getModuleName
  S.intClass n public (inherit p) stVars methods

With buildClass returning type Sclass r , and getModuleName being of type State FileState String. This failed at line n <- getModuleName. Why is the correct answer n <-zoom lensCStoFS getModuleName?

Is it because every monadic action inside of the do statement of build class must be of type “State ClassState r”, hence to execute a monadic action of type “State FileState String” we must first zoom into the FileState of the larger ClassState.

This is why a function much as buildModule which is a FileState does not need a lens to set its name.

buildModule n is ms cs= do 
    modify (setModuleName n)

My second question is related to do statements and states in general. For example in the buildClass example, what state is the lens “lensCStoFS” lensing to, is it the initial file state? For example the _1 lens zooms into the state (1,2) in here
``' (5,2) ^. _1``` (prints out “5”),
but in the buildClass function there is no state specified and a lens seems to be enough.

[bmaclach]

@muhammadaliog3

Is it because every monadic action inside of the do statement of build class must be of type “State ClassState r”, hence to execute a monadic action of type “State FileState String” we must first zoom into the FileState of the larger ClassState.

This is why a function much as buildModule which is a FileState does not need a lens to set its name.

Yes, exactly right. By calling zoom, we are changing getModuleName from State FileState String to State ClassState String, but the function still needs to know how to get the original FileState, so that's why zoom also takes a lens parameter.

My second question is related to do statements and states in general. For example in the buildClass example, what state is the lens “lensCStoFS” lensing to, is it the initial file state? For example the _1 lens zooms into the state (1,2) in here
``' (5,2) ^. _1``` (prints out “5”),
but in the buildClass function there is no state specified and a lens seems to be enough.

lensCStoFS is lensing to the FileState of the current ClassState, wherever it is used. The current ClassState would be the initial ClassState after undergoing any of the modify steps that have been evaluated before the n <- zoom lensCStoFS getModuleName line in buildClass. In this case, it was very important that a modify (setModuleName n) was evaluated before buildClass.

Hopefully this is making some sense. The State monad can be really tricky (I had a really hard time with it when I was first writing this code!), but once you develop an understanding of how it works, it gets easier.

[JacquesCarette]

BTW, it's always a good idea to put the words "closes # NNNN" in the PR text. When I merge it, it will automatically close the issue!

[smiths]

@Nathaniel-Hu, please add to the Contributor's Guide (#2136) the advice from @JacquesCarette about automatically closing an issue with a PR. You can just add the advice to the wiki. There is no need for a local copy under version control.

[Nathaniel-Hu]

Ok then. @smiths I previously added an entry about linking issues in PRs from my discussions with @bmaclach, but I've made a few changes to that entry. Please review it for clarity/correctness.

https://github.com/JacquesCarette/Drasil/blob/Issue%232136/People/Nathaniel/ProposedContributorGuide.md#github

[bmaclach]

@Nathaniel-Hu Unrelated to your above, question, but I just noticed under "Merging Pull Requests", the "Dependent Changes" point is outdated. I think you should replace it with your 4th bullet point under "Creating Pull Requests".

[Nathaniel-Hu]

@bmaclach, I have merged the two points together as an entry under "Creating Pull Requests". Please review it for clarity/accuracy. Thanks!

https://github.com/JacquesCarette/Drasil/blob/Issue%232136/People/Nathaniel/ProposedContributorGuide.md#github

[bmaclach]

@Nathaniel-Hu The wording is good in my opinion, but I think you should link to this comment: #2157 (comment) either instead of or in addition to the comment you are already linking to.

[smiths]

I agree with linking the comment mentioned by @bmaclach above.