-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
idempotent command and code cleanup #648
Conversation
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.
Sorry, it's bit hard to review with all these style changes mixed with logic changes. Could you divide them into separate commits?
Hi Ailrun, I would like to help but I would not know where to start to separate it in different commits, it's a single file and all changes are closely related. Maybe it would be simpler to review directly the new code? It's a very short module: Apart from the code refactoring (mainly getting rid of dead code and simplifying the 'actions' function to 'action') the main difference is that 'action', that calculates what is to be done, is called every time the codeLens is used by the user, rather than just at CodeLens creation time. This avoids the problem of the user clicking multiple times on the CodeLens and ending up with multiple inserts of 'module .. where'. |
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.
@tittoassini i think @Ailrun was talking about the formatting changes in language pragmas and imports.
As a general rule we like to separate semantic from syntactic pr's. But well, being the original author of the plugin, i think we can be flexible in this case 😄
I am gonna approve it, @Ailrun i will wait yours for merging
. unwords | ||
. ("Plugin ModuleName " :) | ||
traceAs :: b -> a -> a | ||
traceAs _ a = a |
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.
Could you clean these debug functions? Other than that, looks good.
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.
Thanks.
But clean them up in what way exactly?
I could redefine them so that they use the standard logger, as in the previous 'out' function.
Would that be ok?
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.
Yes, that and removing the commented one are what I mean.
You are right, next time I will try to be more careful. |
@tittoassini oops, sorry i thought @Ailrun gave his ok 😟 too eager to merge to include it in the next release please do it in a new pr |
This should fix #564
The operation to perform is recalculated before being applied.