Skip to content
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

Merged
merged 1 commit into from
Dec 9, 2020
Merged

Conversation

tittoassini
Copy link
Contributor

This should fix #564

The operation to perform is recalculated before being applied.

Copy link
Member

@Ailrun Ailrun left a 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?

@tittoassini
Copy link
Contributor Author

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:
https://github.com/tittoassini/haskell-language-server/blob/idem/plugins/default/src/Ide/Plugin/ModuleName.hs

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'.

Copy link
Member

@jneira jneira left a 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
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@tittoassini
Copy link
Contributor Author

@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 thos case 😄
I am gonna approve it, @Ailrun i will wait yours for merging

You are right, next time I will try to be more careful.

@jneira jneira merged commit df8845d into haskell:master Dec 9, 2020
@tittoassini
Copy link
Contributor Author

@jneira I see that this branch has been merged, should I then make a different PR for @Ailrun proposed changes to the traceAs function?

@jneira
Copy link
Member

jneira commented Dec 9, 2020

@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

@tittoassini tittoassini deleted the idem branch December 9, 2020 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CodeLens provided by ModuleName does not disappear properly
3 participants