Drasil icon indicating copy to clipboard operation
Drasil copied to clipboard

Split up drasil-gool/Drasil into GOOL, GProc, and Shared folders.

Open sarrasoussia opened this issue 8 months ago • 17 comments

Contributes to #3934

Splitting up drasil-gool/Drasil into GOOL, GProc, and Shared folders.

sarrasoussia avatar May 15 '25 17:05 sarrasoussia

Looks like a good start!

I noticed, it looks like you left everything in the LanguageRenderer folder under Shared. That doesn't seem right to me, since some of these files are (or should be, in some cases) OO-only or Proc-only.

  • The Java, C#, C++, Python, and swift renderers are all OO-only, and the Julia renderer is Proc-only
  • AbstractProc.hs is Proc-only
  • CLike.hs can likely stay here; as its title suggests it has to do with "similarity to C" rather than "OO vs Procedural"
  • CommonPseudoOO.hs should be OO-only based on the name, but I wouldn't be surprised if the Julia renderer uses it. Maybe take a look at what functionality it provides and whether it's worth moving to GOOL or leaving in Shared.
  • From a quick glance at the others, I believe they are all shared as well.

There might be a good reason you didn't move those files I mentioned; it just seems to me that moving them (and taking a look at CommonPseudoOO.hs) would make more organizational sense.

B-rando1 avatar May 15 '25 22:05 B-rando1

I didnt remove the helpers from the CommonOO file just in case ( which is causing the HLint error ). If the new file is fine Ill clean it.

sarrasoussia avatar May 16 '25 18:05 sarrasoussia

Also, plenty of import errors, Im working on them

sarrasoussia avatar May 16 '25 19:05 sarrasoussia

A (now deleted / edited) comment said:

Do you think it is a good idea to split the CommonPseudoOO file into CommonOO, CommonDocs, CommonTypes.. ? This way I can more the OO specific ones to GOOL

That does seem like a good idea. to me.

JacquesCarette avatar May 17 '25 14:05 JacquesCarette

Also: I'll do a full review once the CI passes.

JacquesCarette avatar May 17 '25 14:05 JacquesCarette

After taking a better look at CommonPseudoOO, it definitely seems feasible to split it up in some way. It's split into commented sections that state which renderers use each one, which should make it easier. If nothing better works (though I like your idea to split it up based on Docs, OO, Types, etc), you could move everything used by Julia into a new file (e.g. CommonShared or something), and move the rest of the original file into the GOOL folder.

B-rando1 avatar May 17 '25 15:05 B-rando1

Concerning CommonPseudoOO.hs, I think it should remain in the Shared folder. The Julia renderer (and others) rely on several of its helper functions.

image I got this error that I couldnt fix. It keeps showing despite the fact that I included it in Drasil.Shared.InterfaceCommon

sarrasoussia avatar May 21 '25 20:05 sarrasoussia

Please push your partial work (that gives the above error) to a branch onto the repo. That way I can go into the details and see what the problem is.

JacquesCarette avatar May 21 '25 20:05 JacquesCarette

I didn't notice it was missing, probably due to eye strain from too much screen time.

sarrasoussia avatar May 21 '25 20:05 sarrasoussia

Even after fixing one error, hundreds more appear, a lesson in patience.

sarrasoussia avatar May 21 '25 20:05 sarrasoussia

I guess misplacements caused the errors. I am trying to correct that.

sarrasoussia avatar May 21 '25 20:05 sarrasoussia

How is it coming along?

This is perhaps also a lesson in doing refactors in small pieces instead of trying to jump to the desired end-state all at once. [If you're stuck, it might even be worth starting from scratch but with all your experience, and redo it in small pieces.]

JacquesCarette avatar May 23 '25 14:05 JacquesCarette

I have successfully separated the common functions from CommonPseudoOO.hs used by Julia into the Common.hs file.

sarrasoussia avatar May 27 '25 19:05 sarrasoussia

Regarding issue https://github.com/JacquesCarette/Drasil/issues/3934#issue-2478660676:

  • drasil-gool should be renamed to drasil-codegen (per Dr. Carette's suggestion to reflect GOOL and GProc collectively).
  • drasil-gool/Drasil has been split into GOOL, GProc, and Shared folders.
  • Common functions from CommonPseudoOO.hs have been moved to Common.hs.
  • Awaiting feedback on suggestion at https://github.com/JacquesCarette/Drasil/issues/3934#issuecomment-2913925980 (related to issue https://github.com/JacquesCarette/Drasil/issues/4083).

sarrasoussia avatar May 27 '25 20:05 sarrasoussia

Oh? Why was the PR closed? Was that an accident?

balacij avatar May 27 '25 22:05 balacij

I came here to ask the same question!

JacquesCarette avatar May 28 '25 19:05 JacquesCarette

This PR looks very close to ready! @sarrasoussia Can you just confirm if this PR closes that issue or if it only contributes to it? Would there be any work left to address that issue? It looks like you've addressed everything to me, but I want to double-check 😄

Also, @JacquesCarette has some comments that you should address regarding imports. Now that I've merged main into this branch as well, you might see the CI fail because of unused dependencies, name shadowing and such that you will need to address.

balacij avatar Jun 06 '25 20:06 balacij

@balacij I just need to review the imports and write the ones used explicitly

sarrasoussia avatar Jun 09 '25 15:06 sarrasoussia

Capture d’écran 2025-06-09 à 12 08 28

Why did we need to create the variable classVar then assign it to extClassVar ? Isnt it more appropriate to just write extClassVar = CP.classVar R.classVar. It is creating an ambiguous occurrence of ‘classVar’ error.

sarrasoussia avatar Jun 09 '25 16:06 sarrasoussia

Instances in Haskell don't work that way. The left hand side (LHS) are in the context of defining methods of OOVariableSym and the rhs from the current context. So classVar = CP.classVar R.classVar is not creating a variable, it is assigning to a (required) method.

JacquesCarette avatar Jun 09 '25 17:06 JacquesCarette