Made constDecDef capitalize constants in Python
#3858
Conversation
It capitalizes them, and if the capitalized name shadows another name in the scope it throws an error.
It's still broken, though 😔
I added an extra boolean parameter to `staticVar` (disregarded by all but the Python renderer) to say if the static variable is a variable or a constant. Then I used that to uppercase static constants in the Python renderer. The one thing I don't like about this is that it requires static variables to be slightly dishonest about their contents: their name is as given, while their contents are uppercase.
|
I got everything working roughly how it should. I'll just restate my couple of comments from the issue:
|
B-rando1
requested review from
JacquesCarette,
bmaclach,
smiths,
samm82 and
cd155
as code owners
| available <- varNameAvailable n | ||
| if available | ||
| then varDecDef newConst e | ||
| else error "Cannot safely capitalize constant." |
There was a problem hiding this comment.
Do we want this to be an error? We could just warn the user and potentially just use the lowercase version of it (assuming that it is available)…
There was a problem hiding this comment.
That's a good point. It would be better to do it that way. The only thing I'm not sure about is, how do we keep track of whether or not the renderer used the modified name? When the constant function shows up later, it needs to know whether or not to modify the variable name.
- We could have it infer whether the modified or unmodified name was used by checking
varNameAvailable (variableName v')- if the original name was used, we can probably assume that it was used for the constant in question, else we must have modified it. This would work, but it feels a little sketchy to me. - A better way might be to have a mapping from GOOL variable name to rendered variable name floating around in the State. Then in the
constantfunction, we could look up the name to use, and not have to worry about any misplaced assumptions. This would be a bit of extra work to implement, though.
What are your thoughts? I'd rather not do option 2 until @JacquesCarette can take a look. Would it be better to do option 1 for now, as it should be a small change?
There was a problem hiding this comment.
As a side note, what's the best way to print a warning in Haskell? I'm not actually sure if there's anywhere else in GOOL that currently prints warnings. It seems like a shame to wrap all of GOOL in IO String just for this, is there a better way?
There was a problem hiding this comment.
Honestly, I'm thinking that "error vs. warning" may be a variability we'd want to introduce at some point. Since this is likely going to be a bigger change, I would create a new issue for tracking our design decisions and working on it in a separate PR (if at all!)
No idea about your warning question 😅
| AbsTol = 1.0e-10 | ||
| RelTol = 1.0e-10 | ||
| ABSTOL = 1.0e-10 | ||
| RELTOL = 1.0e-10 |
There was a problem hiding this comment.
What I'd really expect here is ABS_TOL and REL_TOL, to preserve the separation between the two parts of the variable name. Is there a reasonably straightforward way of doing this? (If not, this is likely getting "into the weeds"; NoPCM explicitly uses underscores for its variable names, so we may just want to do that for Projectile as well, potentially enforcing variable names to be all lowercase?)
There was a problem hiding this comment.
That's a good point! It should be possible to do better. Off the top of my head, I think if we just insert a _ wherever there's a lowercase letter on the left and an uppercase letter on the right, it would match what we expect in just about every case. I'll see what functions are available, and get something working.
There was a problem hiding this comment.
I made new function to do the renaming, and it handled the few examples we have well.
Now it inserts underscores at boundaries of camelCase names. The algorithm to do so is probably not ideal, as it heavily relies on pattern-matching when higher-order functions might be better. It works (at least for now), though.
code/drasil-gool/lib/GOOL/Drasil/LanguageRenderer/PythonRenderer.hs
Outdated
Show resolved
Hide resolved
|
Some conflicts need resolved before I can merge. |
|
Just to summarize the developments, there are a couple of things that aren't ideal with the current state of this implementation:
Based on this, what do we want to do with this PR? Is it good enough to merge in its current form, or would it be better to wait for now, and get these safety checks in place first? |
|
More conflicts to resolve. I am happy to pull this in as-is, and defer the resolution of name conflicts to a different pass (i.e. that conflicts throw a hard error for now). |
It capitalizes them, and if the capitalized name shadows another name in the scope it throws an error.
You can see the changes to
HelloWorld.py. I also did a test where I declared a second constantMYCONSTANT(capitalized in the GOOL code), and the renderer threw an error, as intended.Closes #2179