protofire / solhint

Solhint is an open-source project to provide a linting utility for Solidity code.
https://protofire.github.io/solhint/
MIT License
1.03k stars 160 forks source link

Feature request: Allow to set the path for the plugins #206

Open juanfranblanco opened 4 years ago

juanfranblanco commented 4 years ago

Hi I hope you are all well,

Vscode is not able to load the plugins as these will not be packaged with the extension, to workaround this it will be ideal if a path could be passed to load the plugins together with the list of names.

https://github.com/protofire/solhint/blob/32ec781509c9b76db325d0c2db91c8c478441d6a/lib/rules/index.js#L53

This is how it is handled in prettier.

Suggestion: The solhint.json file could include a path or allow to set this by the ide by setting the path to be the same as the current project node_modules.

Reference: https://github.com/juanfranblanco/vscode-solidity/issues/169

fvictorio commented 4 years ago

Hi @juanfranblanco, sorry for not responding before. I'm not 100% sure I understand what would work for you. In the linked issue you suggest having a flag similar to prettier's --plugin-search-dir, is that correct? If we implement that, would that solve the issue for you?

juanfranblanco commented 4 years ago

Don't worry, responding issues is really hard and takes time.

You are right that is how it works with prettier. So the extension could do the usual magic of letting the user set it on the solhint file, in the extension user settings or if nothing is set, use the the default node_modules in the workspace.

This is how I have integrated with prettier if it helps: https://github.com/juanfranblanco/vscode-solidity/blob/master/src/formatter/prettierFormatter.ts#L17-L18

fvictorio commented 4 years ago

I see, makes sense. Will try that approach then, thanks :+1:

I guess you are using solhint as an executable binary, right? In version 3.0 we tried to also properly export it as a "library", so maybe you want to try this. That's not documented yet, but it's just:

const { processStr } = require('solhint')

processStr(source, config, filename)

Both config and filename are optional. In this case, I guess you'd pass the pluginSearchDirs options there.

fvictorio commented 4 years ago

Oh, and we should document what is returned by that function :man_facepalming: I'll try to do it when I have some time, but for the time being you can probably figure it out by inspecting it :sweat_smile: Let me know if you have any questions about that.

juanfranblanco commented 4 years ago

Solium is used as follows: https://github.com/juanfranblanco/vscode-solidity/blob/master/src/linter/solium.ts it was contributed by yourselves :). So all the integration is done internally.

fvictorio commented 4 years ago

This is solhint, not solium :smile:

juanfranblanco commented 4 years ago

Oh my!! Sorry :)

juanfranblanco commented 4 years ago

@fvictorio it seems that we are having issues with extends too [solhint:recommended] will this be related, it used to work before.

Jaime-Iglesias commented 4 years ago

Any update on this issue ? I'd love to see custom plugins and the recommended rule set working on the extension :D

friendly ping @fvictorio

fvictorio commented 4 years ago

Not yet, hopefully this week.

fvictorio commented 4 years ago

Hey @juanfranblanco, could you try installing Solhint from github, using the plugin-search-dir branch, and see if this works for you?

You should call it like this:

processStr(solidityCode, config, {
  pluginSearchDir: '/path/to/dir'
})

Where solidityCode is a string with the code to lint, config is an object like the one in .solhint.json, and pluginSearchDir is the path to where the node_modules directory with the plugins is located. That means that you should use /path/to/plugins, not /path/to/plugins/node_modules.

fvictorio commented 4 years ago

I just did the same change for shareable configs (aka extends) too. Same branch, so you should be able to test it.

Of course, that means that pluginSearchDir is a bad name, I will think of something better (extensionsDir?), but first I want to see if it works :smile:

Jaime-Iglesias commented 4 years ago

@juanfranblanco friendly ping

juanfranblanco commented 4 years ago

@Jaime-Iglesias sorry a bit overboard at the moment, next week i will have some time for this ;)

Jaime-Iglesias commented 4 years ago

@Jaime-Iglesias sorry a bit overboard at the moment, next week i will have some time for this ;)

Yeah, don't worry - just wanted to make sure you had seen it - Stay safe !

fvictorio commented 4 years ago

Hey @juanfranblanco, did you have a chance to test this?

juanfranblanco commented 4 years ago

@fvictorio no I have not had time.. did you included it in the current 3.0.0 release?

fvictorio commented 4 years ago

No, it's a little risky so I wanted to validate it first.

I don't use VSCode, but how hard is for me to test it myself? I guess I would need to clone your extension, add this change, and load it locally in the editor. Is that correct?

juanfranblanco commented 4 years ago

yeah.. just clone the extension and run it :) now obviously you need to reference your package to test the change, it will be great if you could do it as you know best what to expect and how to use that package. If you need any help to get setup let me know.

Jaime-Iglesias commented 4 years ago

Hi all!

Has there been any progress regarding this issue ?

fvictorio commented 4 years ago

@Jaime-Iglesias I'm working on it, I hope to have it fixed on these days!

@juanfranblanco I started looking into this but I ran into an issue. I don't know anything about developing vscode extensions, so I might be asking very silly questions.

I edited solhint.ts to receive a extensionPath: string in the constructor, so that that can be used when calling linter.processStr. The problem is, SolhintService is instantiated in server.ts and that file doesn't have access to context.extensionPath. This doesn't happen with formatDocument, that is used in extension.ts and has access to that context. Could you give me a pointer here?

This is the branch I'm working on: https://github.com/fvictorio/vscode-solidity/commit/53d8f4c7d371f8744431e2a3cea580b5721996b9 Of course, for that to work you have to link solhint in the plugin-search-dir branch.

Jaime-Iglesias commented 3 years ago

Hi all - hope everyone is doing well!

Has there been any progress on this ?

fvictorio commented 3 years ago

Hey @Jaime-Iglesias, not from me, I got stuck with what I mentioned in my last comment and haven't really worked on this since then, and I don't really know when I'll have time to do it :slightly_frowning_face:

Jaime-Iglesias commented 3 years ago

Hey @Jaime-Iglesias, not from me, I got stuck with what I mentioned in my last comment and haven't really worked on this since then, and I don't really know when I'll have time to do it

No worries - I think I'll endup just ditching the solidity extension completely and switch to a different approach, solhint is too useful to let go :D

Totally unrelated, just subscribed to your "exploring Ethereum", keep it up !