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

Add jupyfmt formatter #403

Closed
wants to merge 6 commits into from
Closed

Add jupyfmt formatter #403

wants to merge 6 commits into from

Conversation

kpj
Copy link
Contributor

@kpj kpj commented Apr 6, 2021

This PR adds support for the Jupyter notebook formatter jupyfmt.

@kpj kpj requested a review from nvuillam as a code owner April 6, 2021 15:02
@kpj
Copy link
Contributor Author

kpj commented Apr 6, 2021

For some reason, ./build.sh does not add new entries to .automation/generated/linter-helps.json and .automation/generated/linter-versions.json.
Do you have any idea why?

I assume that the (automatically generated) links https://github.com/nvuillam/mega-linter/tree/master/docs/descriptors/jupyter.md#readme and https://github.com/nvuillam/mega-linter/tree/master/docs/descriptors/jupyter_jupyfmt.md#readme being dead is not a big issue.

@nvuillam
Copy link
Member

nvuillam commented Apr 6, 2021

Congratulations, you got very well how MegaLinter architecture works :)

  1. Help and version work when they can be retrieved
    That's lated to the 2 CI tests failing :/
FAILED megalinter/tests/test_megalinter/linters/jupyter_jupyfmt_test.py::jupyter_jupyfmt_test::test_get_linter_help
FAILED megalinter/tests/test_megalinter/linters/jupyter_jupyfmt_test.py::jupyter_jupyfmt_test::test_get_linter_version

It can be solved by defining cli_help_arg_name and cli_version_args_name in descriptor

  1. As your package is quite new, and Mega-Linter docker images already heavy, may I suggest that you create a MegaLinter plugin ?
    I could implement a system for "Official plugins", so when running MegaLinter and detecting Jupyter Notebook files, it would suggest to use the plugin

  2. Check format is nice, auto-format is better ^^ So if you can auto-apply formatting, you could define something like black descriptor

    cli_lint_fix_arg_name: "--megalinter-fix-flag" # Workaround for Mega-Linter
    cli_lint_fix_remove_args:
      - "--diff"
      - "--check"

@kpj
Copy link
Contributor Author

kpj commented Apr 7, 2021

Congratulations, you got very well how MegaLinter architecture works :)

Thanks a lot! MegaLinter having a very nice architecture to work with helped quite a bit :-)

  1. Help and version work when they can be retrieved
    That's lated to the 2 CI tests failing :/
FAILED megalinter/tests/test_megalinter/linters/jupyter_jupyfmt_test.py::jupyter_jupyfmt_test::test_get_linter_help
FAILED megalinter/tests/test_megalinter/linters/jupyter_jupyfmt_test.py::jupyter_jupyfmt_test::test_get_linter_version

It can be solved by defining cli_help_arg_name and cli_version_args_name in descriptor

That makes sense, thanks!
If missing these parameters makes the tests fail, shouldn't they also be required?

  1. As your package is quite new, and Mega-Linter docker images already heavy, may I suggest that you create a MegaLinter plugin ?
    I could implement a system for "Official plugins", so when running MegaLinter and detecting Jupyter Notebook files, it would suggest to use the plugin

Sure, I'd then close this PR, I guess.
I would simply have to add the descriptor to the jupyfmt repository and add it with, e.g., PLUGINS: ["https://raw.githubusercontent.com/kpj/jupyfmt/master/.jupyfmt.megalinter-descriptor.yml"] (i.e. it could be a hidden file), right?

  1. Check format is nice, auto-format is better ^^ So if you can auto-apply formatting, you could define something like black descriptor

Certainly! What exactly is the purpose of --megalinter-fix-flag?

@nvuillam
Copy link
Member

Megalinter fix flag is a workaround for when there is no --fix flag available for a linter (often the case for formatters)
When met (megalinter adds it when there is no fix flag but APPLY_FIXES is true), it removes the --check argument, you can look at black, markdownlint, etc... descriptors :)

@nvuillam
Copy link
Member

About hidden file or not for plugin... all it requires is to be downloadable from http :)

@kpj
Copy link
Contributor Author

kpj commented Apr 14, 2021

Thanks for the feedback :-)

When adding the plugin, MegaLinter has very specific requirements (Plugin descriptors must follow the format https://**/mega-linter-plugin-**/**.mega-linter-descriptor.yml) which means I need to put the descriptor in a specific sub-directory which cannot be hidden. Does this requirement have a benefit?

When I then try to run MegaLinter with npx mega-linter-runner and the configuration

ENABLE_LINTERS:
  - JUPYTER_JUPYFMT
PLUGINS:
  - https://raw.githubusercontent.com/kpj/jupyfmt/master/mega-linter-plugin-jupyfmt/jupyfmt.megalinter-descriptor.yml
FORMATTERS_DISABLE_ERRORS: false

it crashes with FileNotFoundError: [Errno 2] No such file or directory: 'jupyfmt', despite having

linters:
  # JUPYFMT
  - linter_name: jupyfmt
[..]
    install:
      pip:
        - jupyfmt

in the descriptor. Is this due to the limitation For now, the only install attributes managed are dockerfile instructions starting by RUN mentioned in the documentation?

@nvuillam
Copy link
Member

  • About hidden ... yes I added the folder name requirement so I can more easily track plugins on GitHub (in case someone malignant would add some dangerous plugin ^^ )

  • please use the following :

    install:
      dockerfile:
        - RUN pip install jupyfmt

@kpj
Copy link
Contributor Author

kpj commented Apr 14, 2021

  • About hidden ... yes I added the folder name requirement so I can more easily track plugins on GitHub (in case someone malignant would add some dangerous plugin ^^ )

Fair enough :-)

  • please use the following :
    install:
      dockerfile:
        - RUN pip install jupyfmt

I updated the descriptor accordingly, but the crash remains (even after deleting previous local MegaLinter Docker images).
Here's the whole error message:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/megalinter/run.py", line 15, in <module>
    linter.run()
  File "/megalinter/MegaLinter.py", line 127, in run
    self.process_linters_serial(active_linters, linters_do_fixes)
  File "/megalinter/MegaLinter.py", line 172, in process_linters_serial
    linter.run()
  File "/megalinter/Linter.py", line 434, in run
    return_code, stdout = self.process_linter(file)
  File "/megalinter/Linter.py", line 550, in process_linter
    return_code, return_output = self.execute_lint_command(command)
  File "/megalinter/Linter.py", line 592, in execute_lint_command
    process = subprocess.run(
  File "/usr/local/lib/python3.8/subprocess.py", line 493, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/local/lib/python3.8/subprocess.py", line 858, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/local/lib/python3.8/subprocess.py", line 1704, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'jupyfmt'

@kpj
Copy link
Contributor Author

kpj commented May 12, 2021

@nvuillam: I have been playing around with this a bit, but still haven't found a solution :-( Do you have a suggestion in which direction I could try further?

@nvuillam
Copy link
Member

@kpj > The error just says it can not find jupyfmt, but your plugin linter should be installed there -> https://github.com/nvuillam/mega-linter/blob/db8235e423c61b1f5c26a55c05ec744c8ffa8fc3/megalinter/plugin_factory.py#L71

Do you see it installed in the full log ? ( do you have a link to such CI log ? )

@kpj
Copy link
Contributor Author

kpj commented May 12, 2021

Do you see it installed in the full log ? ( do you have a link to such CI log ? )

I am currently running it locally via Docker by executing npx mega-linter-runner.
Here's the terminal output I get:

terminal output
[config] /tmp/lint/.mega-linter.yml + Environment variables
----------------------------------------------------------------------------------------------------
------------------------------------------- Mega-Linter --------------------------------------------
----------------------------------------------------------------------------------------------------
 - Image Creation Date: 2021-04-30T13:57:03Z
 - Image Revision: d76d551
 - Image Version: v4
----------------------------------------------------------------------------------------------------
The Mega-Linter documentation can be found at:
 - https://nvuillam.github.io/mega-linter
----------------------------------------------------------------------------------------------------
GITHUB_REPOSITORY:
GITHUB_SHA:
GITHUB_TOKEN:
GITHUB_RUN_ID:


[Plugins] Loaded plugin descriptor /megalinter-descriptors/jupyfmt.megalinter-descriptor.yml from https://raw.githubusercontent.com/kpj/jupyfmt/master/mega-linter-plugin-jupyfmt/jupyfmt.megalinter-descriptor.yml
[Plugins] Successful initialization of JUPYTER plugins
Skipped linters: ANSIBLE_ANSIBLE_LINT, ARM_ARM_TTK, BASH_EXEC, BASH_SHELLCHECK, BASH_SHFMT, CLOJURE_CLJ_KONDO, CLOUDFORMATION_CFN_LINT, COFFEE_COFFEELINT, COPYPASTE_JSCPD, CPP_CPPLINT, CSHARP_DOTNET_FORMAT, CSS_SCSS_LINT, CSS_STYLELINT, C_CPPLINT, DART_DARTANALYZER, DOCKERFILE_DOCKERFILELINT, DOCKERFILE_HADOLINT, EDITORCONFIG_EDITORCONFIG_CHECKER, ENV_DOTENV_LINTER, GHERKIN_GHERKIN_LINT, GIT_GIT_DIFF, GO_GOLANGCI_LINT, GO_REVIVE, GRAPHQL_GRAPHQL_SCHEMA_LINTER, GROOVY_NPM_GROOVY_LINT, HTML_HTMLHINT, JAVASCRIPT_ES, JAVASCRIPT_PRETTIER, JAVASCRIPT_STANDARD, JAVA_CHECKSTYLE, JSON_ESLINT_PLUGIN_JSONC, JSON_JSONLINT, JSON_PRETTIER, JSON_V8R, JSX_ESLINT, KOTLIN_KTLINT, KUBERNETES_KUBEVAL, LATEX_CHKTEX, LUA_LUACHECK, MARKDOWN_MARKDOWNLINT, MARKDOWN_MARKDOWN_LINK_CHECK, MARKDOWN_MARKDOWN_TABLE_FORMATTER, MARKDOWN_REMARK_LINT, OPENAPI_SPECTRAL, PERL_PERLCRITIC, PHP_BUILTIN, PHP_PHPCS, PHP_PHPSTAN, PHP_PSALM, POWERSHELL_POWERSHELL, PROTOBUF_PROTOLINT, PUPPET_PUPPET_LINT, PYTHON_BLACK, PYTHON_FLAKE8, PYTHON_ISORT, PYTHON_PYLINT, RAKU_RAKU, RST_RSTCHECK, RST_RSTFMT, RST_RST_LINT, RUBY_RUBOCOP, RUST_CLIPPY, R_LINTR, SALESFORCE_SFDX_SCANNER_APEX, SALESFORCE_SFDX_SCANNER_AURA, SALESFORCE_SFDX_SCANNER_LWC, SCALA_SCALAFIX, SNAKEMAKE_LINT, SNAKEMAKE_SNAKEFMT, SPELL_CSPELL, SPELL_MISSPELL, SQL_SQLFLUFF, SQL_SQL_LINT, SWIFT_SWIFTLINT, TEKTON_TEKTON_LINT, TERRAFORM_TERRAGRUNT, TERRAFORM_TERRASCAN, TERRAFORM_TFLINT, TSX_ESLINT, TYPESCRIPT_ES, TYPESCRIPT_PRETTIER, TYPESCRIPT_STANDARD, VBDOTNET_DOTNET_FORMAT, XML_XMLLINT, YAML_PRETTIER, YAML_V8R, YAML_YAMLLINT
To receive reports as email, please set variable EMAIL_REPORTER_EMAIL
Listing all files in directory [/tmp/lint], then filter with:
- File extensions: .ipynb
Kept [12] files on [37034] found files

+----MATCHING LINTERS--+----------+----------------+------------+
| Descriptor | Linter  | Criteria | Matching files | Format/Fix |
+------------+---------+----------+----------------+------------+
| JUPYTER    | jupyfmt | .ipynb   | 12             | no         |
+------------+---------+----------+----------------+------------+

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/megalinter/run.py", line 15, in <module>
    linter.run()
  File "/megalinter/MegaLinter.py", line 127, in run
    self.process_linters_serial(active_linters, linters_do_fixes)
  File "/megalinter/MegaLinter.py", line 172, in process_linters_serial
    linter.run()
  File "/megalinter/Linter.py", line 434, in run
    return_code, stdout = self.process_linter(file)
  File "/megalinter/Linter.py", line 550, in process_linter
    return_code, return_output = self.execute_lint_command(command)
  File "/megalinter/Linter.py", line 592, in execute_lint_command
    process = subprocess.run(
  File "/usr/local/lib/python3.8/subprocess.py", line 493, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/local/lib/python3.8/subprocess.py", line 858, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/local/lib/python3.8/subprocess.py", line 1704, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'jupyfmt'

I mostly just notice the following as positive messages:

[Plugins] Loaded plugin descriptor /megalinter-descriptors/jupyfmt.megalinter-descriptor.yml from https://raw.githubusercontent.com/kpj/jupyfmt/master/mega-linter-plugin-jupyfmt/jupyfmt.megalinter-descriptor.yml
[Plugins] Successful initialization of JUPYTER plugins

The file report/mega-linter.log does also not show more information. Can I get a more verbose output somehow?

@nvuillam
Copy link
Member

mmm maybe there is a bug when the install is withing the linter

Please can you try with
LOG_LEVEL=DEBUG

and in your descriptor, put the install at the root YAML level, not under the linters element ?

@nvuillam
Copy link
Member

I see where is the bug

image

And it's on my side ^^

With the workaround of my previous post it should be ok for your case until I fix the bug

@nvuillam
Copy link
Member

image

Here, it should be if "install" in linter_description, not descriptor :/

kpj added a commit to kpj/jupyfmt that referenced this pull request May 12, 2021
@kpj
Copy link
Contributor Author

kpj commented May 12, 2021

Aha, good catch!

I now put

install:
  dockerfile:
    - RUN pip install jupyfmt

at the root of my YAML descriptor.

Running the linter then crashes with:

[Plugins] Loaded plugin descriptor /megalinter-descriptors/jupyfmt.megalinter-descriptor.yml from https://raw.githubusercontent.com/kpj/jupyfmt/master/mega-linter-plugin-jupyfmt/jupyfmt.megalinter-descriptor.yml
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/megalinter/run.py", line 9, in <module>
    linter = megalinter.Megalinter({"cli": True})
  File "/megalinter/MegaLinter.py", line 89, in __init__
    plugin_factory.initialize_plugins()
  File "/megalinter/plugin_factory.py", line 21, in initialize_plugins
    install_plugin(descriptor_file)
  File "/megalinter/plugin_factory.py", line 59, in install_plugin
    process_install(descriptor["install"])
  File "/megalinter/plugin_factory.py", line 77, in process_install
    commands += [
  File "/megalinter/plugin_factory.py", line 78, in <listcomp>
    command.replace("RUN ").replace(" \\\n", "\n")
TypeError: replace expected at least 2 arguments, got 1

It seems like the first call to replace lacks a replacement value. It should probably just be replace("RUN ", "")?

@nvuillam
Copy link
Member

You're right >_<
Wait, i'll publish a quick fix

@nvuillam
Copy link
Member

I published a debug, now both cases are supposed to work :)
( just wait for new @latest image to be built ^^ )

kpj added a commit to kpj/jupyfmt that referenced this pull request May 12, 2021
@kpj
Copy link
Contributor Author

kpj commented May 13, 2021

Thank you so much! It now works for me :-)

I also mentioned Mega-Linter in jupyfmt's readme and started using it one of our workflows.

@nvuillam
Copy link
Member

Great :)
I'll add your plugin in the plugins list in Mega-Linter documentation :)
And the day i'll have some time, i'll even generate plugin documentation so it can be hosted on my side and/or yours ^^

@nvuillam nvuillam closed this May 13, 2021
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.

None yet

2 participants