-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #206, allow primes in names #437
Fix #206, allow primes in names #437
Conversation
@@ -42,7 +44,7 @@ import qualified AST.Meta as Meta | |||
import Identifiers | |||
|
|||
-- the following line of code resolves the standard path at compile time using Template Haskell | |||
standardLibLocation = $((stringE . init) =<< (runIO $ System.Environment.getEnv "ENCORE_BUNDLES" )) | |||
standardLibLocation = $((stringE . init) =<< runIO (System.Environment.getEnv "ENCORE_BUNDLES" )) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR is about refactoring :) you missed:
$((stringE . init) =<< runIO (System.Environment.getEnv "ENCORE_BUNDLES" ))
which can be written as
$(stringE . init =<< runIO (System.Environment.getEnv "ENCORE_BUNDLES" ))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is a feature branch, not a refactoring branch, but I can fix it anyway ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed my point... 😈 this feature branch is a whole refactoring of styling issues.
TopLevel
has 52 additions, from which 14 lines are the actual change. I agree with refactorings that make code faster, cleaner etc, but these changes:
Nam $ encoreName "oneway_msg" ((Ty.getId cls) ++ "_" ++ show mname ++ "_t")
into
Nam $ encoreName "oneway_msg" (Ty.getId cls ++ "_" ++ show mname ++ "_t")
I see the point, I don't complain that I like it more, but it obfuscates the real content.
I have mixed feelings... it's good and bad... a feature that has 500 lines changes were 300 are clean up... maybe the clean up deserves its own PR.
Sorry that I am rambling... PRs where the actual changes are bigger than the clean up are acceptable (this PR goes into that category, more actual code than style refactoring)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point taken! These kind of refactorings are so quick when you have hlint
, I hardly think about doing them anymore. I'll try to keep refactoring closer to the actual changes in the future!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option: a separate commit, which does the final styling clean up. I can look at the meaningful changes first (initial commit) and then the clean up. We do a squash at the end and it looks as a single commit with the benefit of being reviewed for that it does.
I do understand that, in practice, this may be quite difficult though
This commit adds support for using primes in identifiers. It has been tested for variables, type variables, methods, fields, classes, traits and typedefs. The renaming scheme is somewhat intricate to prevent non-clashing identifiers from clashing at the C-level. There is an added test `primeNames.enc`.
Everything looks good. Merging in 15 min! |
This commit adds support for using primes in identifiers. It has been
tested for variables, type variables, methods, fields, classes, traits
and typedefs. The renaming scheme is somewhat intricate to prevent
non-clashing identifiers from clashing at the C-level. There is an added
test
primeNames.enc
.