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

Is it still alive? Some advantages are needed. #7

Open
tolezk opened this issue Mar 4, 2022 · 4 comments
Open

Is it still alive? Some advantages are needed. #7

tolezk opened this issue Mar 4, 2022 · 4 comments

Comments

@tolezk
Copy link

tolezk commented Mar 4, 2022

Hi, @idanpa and thank you for the extension!

So, long story short, I need this to have some additional features:

  • support log parsing when there --showfile option is passed to the checkpatch
  • support a possibility to define a particular cwd for multiroot workspaces to make checkpatch use a config from specific workspace

I'm not a person who familiar with js, ts or vscode-extensions creation. But I can provide a pull-request where I'll try to do my best to implement these features. It may violate typical codestyle or common sense for ts, though.

Now, some details.

The idea behind the first feature is to implement an alternative parseCheckpatchLog() function (or addition to it) which may handle a different output from checkpatch. For comparison:

WARNING:LINUX_VERSION_CODE: LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged
#124: FILE: mac.c:124:
+#if KERNEL_VERSION(5, 8, 1) <= LINUX_VERSION_CODE

- this style of logs is supported for now.

mac.c:124: WARNING:LINUX_VERSION_CODE: LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged
+#if KERNEL_VERSION(5, 8, 1) <= LINUX_VERSION_CODE

- This style I want to be supported (here --showfile option was passed to checkpatch).

About second feature. I think it would be good to define some option that a user can define inside settings.json of a particular workspace, and then extension should use this workspace as cwd, but not the root. I don't know if this possible or some other approach should be choosen. Of course this related to the Checkpatch Selected File feature.

I need some feedback for it from you,
Cheers.

@idanpa
Copy link
Owner

idanpa commented Mar 5, 2022

Hi @tolezk, thanks for opening this issue!

  1. --showfile - seems reasonable to support this flag, feel free to open a PR (need to adjust the regex if showfile arg was given).
  2. It is possible to adjust the cwd of the checkpatch process, but seems overcomplicated to me. checkpatch would run from the root dir of your project, so you can have the .checkpatch.conf file waiting for it in there, alternatively, you can adjust checkpatch.checkpatchArgs on settings.json if you want to adjust the flags for specific workspace/globally.

@tolezk
Copy link
Author

tolezk commented Mar 10, 2022

I've made a PR (#8) to cover 1. '--showfile'.

Now some thoughts about 2: run checkpatch for a particular file.

I think it should be similar to checkpatchCommit() in terms of CWD inheritance etc.
Running a linter from the root workspace is a quite basic use-case because a project workspace layout may be somehow tricky. For example:

vscode_workspace:
|
|--root_workspace (some complex project with many directories and subprojects inside)
|  |--dir1
|  |
|  |--dir2
|  |
|  |--dir3
|  |
|  |--subproject_A
|  |
|  |--subproject_B
|  |
|  |--scripts/checkpatch.pl (this one may be a non-standard somehow modified to cover custom requirements)
|  |--.checkpatch.conf
|  
|--subproject_A_of_root_workspace  
|  |--dir1
|  |
|  |--dir2
|  |
|  |--.checkpatch.conf
|
|--subproject_B_of_root_workspace  
|  |--dir1
|  |
|  |--dir2
|
|--linux
   |--arch
   |
   |--... (all the kernel stuff)
   |
   |--scripts/checkpatch.pl
   |--.checkpatch.conf

Here we have:

  • root_workspace: this contains modified implementation of checkpatch.pl script and its config .checkpatch.conf
  • subproject_A_of_root_workspace: this relies on root_workspace checkpatch.pl, but has own config
  • subproject_B_of_root_workspace: this relies on root_workspace checkpatch.pl and config
  • linux: this has its own well-defined checkpatch.pl and .checkpatch.conf

To keep things playable in even a such case my proposal is to let a user define folder-level settings inside each folder, where required. And it still would keep backward compatibility with the current implementation. And would give the flexibility to achieve these goals.

Some details. Several options (checkpatchPath, checkpatchArgs, exclude) I want to switch to 'resource' scope. Also, I want to introduce a new option useWorkspaceAsCwd with the same scope which would let to consider the folder it applied to as CWD.

@tolezk
Copy link
Author

tolezk commented Mar 10, 2022

I want you to confirm that proposal is ok to you and I will prepare the next PR to implement. Or we may have some discussion about.

@tolezk
Copy link
Author

tolezk commented Mar 13, 2022

Anyway, I've implemented all the stuff. Now preparing a new PR.

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

No branches or pull requests

2 participants