Extract the pre-decl pragma parsing to its own module

[eddiemundo]

The purpose is so that it can be reused by multiple plugins, and ghcide.

[jneira]

Many thanks for the refactoring: it makes sense to me but i would like to know @pepeiborra thoughts about the chosen place and name

[pepeiborra]

Unless you are planning to use this in ghcide, it doesn't seem like a good idea to move it in. In my view code should be moved out of ghcide where possible, in order to shrink the core surface that needs to be maintained and updated.

[jneira]

hmm ok, so we should looking for another place to put this kind of code, useful for several plugins, depending on ghcide and no suitable to be in ghcide itself 🤔

[jneira]

hmm ok, so we should looking for another place to put this kind of code, useful for several plugins, depending on ghcide and no suitable to be in ghcide itself

alternatives:

• put the code in a concrete plugin and make the other ones depending on it: dont like that
• create a new package hls-plugin-common or hls-plugin-utils: dont like it as it increases maintainers work but it seems to be the cleaner one

thoughts @pepeiborra?

[pepeiborra]

alternatives:

• put the code in a concrete plugin and make the other ones depending on it: dont like that

• create a new package hls-plugin-common or hls-plugin-utils: dont like it as it increases maintainers work but it seems to be the cleaner one

thoughts @pepeiborra?

What's the problem with plugins depending on each other?

• if plugins A and B depend on plugin P, only A and B will break if P breaks while the rest of plugins will be fine.
• If we have a plugin-common library that all the plugins depend on, it becomes a piece of core code that needs to be maintained and upgraded just like ghcide

[jneira]

if plugins A and B depend on plugin P, only A and B will break if P breaks while the rest of plugins will be fine.

Well, if the code is generic enough (and we should encourage so), it would not fit naturally in any concrete plugin and will be less discoverable to be used in other ones.

it becomes a piece of core code that needs to be maintained and upgraded just like ghcide

I was thinking in the better place to put it even if it needs more maintainer work. It is one package more between the bunch of existing ones.

[eddiemundo]

The main reason I thought putting this somewhere into its own module was necessary is because the pragmas plugin depends on ghcide, and so if something in ghcide needs some pragmas plugin functionality then there would be a cyclic dependency and it wouldn't work. This might not be the case?

[pepeiborra]

The main reason I thought putting this somewhere into its own module was necessary is because the pragmas plugin depends on ghcide, and so if something in ghcide needs some pragmas plugin functionality then there would be a cyclic dependency and it wouldn't work. This might not be the case?

That's why I said:

Unless you are planning to use this in ghcide, it doesn't seem like a good idea to move it in

Are you planning to use this in ghcide then? If not, best left in the pragmas plugin.

[eddiemundo]

Are you planning to use this in ghcide then? If not, best left in the pragmas plugin.

Oh, yeah, there is an import placement bug outlined in the tests in #2425 that can be solved using functionality in the pragmas plugin, otherwise we'd have to duplicate the logic, or solve it in a different way. The import placement stuff is done in CodeAction.hs which is in ghcide.

[jneira]

thanks for the refactoring