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

Be able to provide documentation URL for custom lint rules #4491

Closed
3 tasks
kmannuru opened this issue Aug 30, 2024 · 19 comments · Fixed by #4843
Closed
3 tasks

Be able to provide documentation URL for custom lint rules #4491

kmannuru opened this issue Aug 30, 2024 · 19 comments · Fixed by #4843
Assignees
Labels
BPMN enhancement New feature or request linting pr welcome We rely on a community contribution to improve this.
Milestone

Comments

@kmannuru
Copy link
Contributor

kmannuru commented Aug 30, 2024

Problem you would like to solve

Camunda Desktop modeler currently displays lint messages. Currently some lint rules are configured to show lint messages along with the documentation url. The documentation urls are currently hardcoded in camunda linting package where the documentation url will only display for some specific linting rules. When implementing custom lint rules currently the linting module doesn't support the configuration to return the custom documentation url.

I would like to create my own lint rules, and define a custom documentation URL with them, so they are picked up by the editor.

Proposed solution

Alternatives considered

See #4491 (comment) for discussion of different approaches.

camunda-modeler repo:

linting repo:

Additional context

No response

@kmannuru kmannuru added the enhancement New feature or request label Aug 30, 2024
@kmannuru
Copy link
Contributor Author

@nikku Please let me know your thoughts on this enhancement? Can I proceed with making the changes and raise PR?

@nikku
Copy link
Member

nikku commented Sep 11, 2024

@kmannuru I think being able to provide custom documentation URLs would be a great enhancement.

Let's consider some options:

  • 🅰️ Provide URL via a dedicated configuration file, and let that file take precedence:
    • 🟢 Allows users to customize documentation for any rule
    • 🟡 Requires additional configuration, next to the rule
    • ❓ Where should the configuration file live, and how shall it be read?
  • 🅱️ Provide URL via rule itself, i.e. a meta property, and populate reports with the relevant meta-data, cf. Add ability to provide custom meta-data with rules bpmn-io/bpmnlint#18
    • 🟢 You can define the URL yourself, right with the rule
    • 🟢 The editor will pick up the URL automatically
    • 🔴 The rule cannot easily be customized
  • 🌵 Continue to inject the URL from the side, through a custom "provider", require overriding that custom provider to customize displayed documentation.

@kmannuru
Copy link
Contributor Author

kmannuru commented Sep 11, 2024

@nikku Thanks for your feedback. Here's is what I plan to do.

  1. Create documentation-url.json configuration file which holds json array of objects containing the documentationUrl mapped to each rule that we plan to override. The file can be empty and clients can override the file. Follows similar implementation approach like flags.json.
    https://github.com/camunda/camunda-modeler/blob/develop/resources/flags.json.

  2. Create documentation-url.js file to read the documentation-url.json contents similar to https://github.com/camunda/camunda-modeler/blob/develop/app/lib/flags.js.

  3. Create documentationUrlMap from flags.js and Pass url map to the new Linter instance.
    https://github.com/camunda/camunda-modeler/blob/develop/client/src/app/TabsProvider.js#L206
    https://github.com/camunda/camunda-modeler/blob/develop/client/src/app/TabsProvider.js#L259

Please let me know your thoughts on this approach and let me know if any issues. Thanks.

@nikku
Copy link
Member

nikku commented Sep 12, 2024

@kmannuru Via the proposed documentation-urls.json file you'd be able to host not only rule related documentation, but potentially re-route any of the provided documentation URLs? I.e. would it allow you override documentation URLs from the properties panel?

capture e6nW10_optimized

How would you distribute the file? Via a custom Camunda Modeler build?

@kmannuru
Copy link
Contributor Author

kmannuru commented Sep 12, 2024

@nikku The changes i'm planning to make will not override the documentation url in the properties panel. The proposed changes will only override the documentation url in the linting error messages.
Currently the documentation urls are being returned by linting package using below function.
https://github.com/camunda/linting/blob/main/lib/utils/documentation.js#L3

We're going to enhance the function to read the documentationUrl map we're going to build and return the mapped documentationUrl configured to the rule.

@nikku
Copy link
Member

nikku commented Sep 13, 2024

The changes i'm planning to make will not override the documentation url in the properties panel.

I got this. But what about simply shipping documentation URL via the rule itself (option 🅱️ here)?

@barmac
Copy link
Collaborator

barmac commented Sep 17, 2024

I just had a look at how ESLint custom rules work, and the documentation URL is part of the rule definition as suggested by @nikku in 🅱️ . I think it makes sense as it is the rule's author who knows where the rule is documented.

@barmac barmac added the needs discussion Needs further discussion label Sep 18, 2024
@barmac
Copy link
Collaborator

barmac commented Sep 18, 2024

@kmannuru given the feedback @nikku and I shared, I'd like to modify the issue to use the solution sketch from 🅱️ in #4491 (comment)
What do you think? Otherwise, we will probably close this issue as wontfix because we are rather not implementing an external documentation JSON for the existing rules.

@nikku nikku changed the title Enhance linter to include custom documentation url for lint error messages Be able to provide documentation URL for custom lint rules Sep 18, 2024
@nikku
Copy link
Member

nikku commented Sep 18, 2024

@barmac I think the feature request is valid, regardless, we just disagree on the direction.

I rephrased the issue so we can leave it open.

@nikku nikku added BPMN linting backlog Queued in backlog pr welcome We rely on a community contribution to improve this. and removed needs discussion Needs further discussion labels Sep 18, 2024
@kmannuru
Copy link
Contributor Author

Thanks for the feedback.
Currently, when I'm creating the custom lint rules, I'm trying to add custom documentation URLs to the messages. But only message text is displayed but not the documentation urls.
I'm basically trying to add documentation url to the linting error message for the custom lint rules. Here's the screenshot of the picture where the custom lint error messages would be displayed.
Screenshot 2024-09-18 at 9 27 23 PM

Just want to make sure if we can accomplish above using the approach B that you've mentioned.
@nikku @barmac

@nikku
Copy link
Member

nikku commented Sep 19, 2024

@kmannuru Yes, what you show will be accomplished using above mechanism, through the following:


Cf. camunda/linting#121 (comment) and camunda/linting#121 (comment) for additional discussion of that "meta-data attached to rules + reports" approach.

@nikku
Copy link
Member

nikku commented Sep 19, 2024

Updated issue based on the confirmed solution sketch.

@kmannuru
Copy link
Contributor Author

kmannuru commented Sep 23, 2024

@nikku @barmac I tried passing the url through metadata in the rule.
https://github.com/camunda/camunda-modeler/blob/develop/resources/plugins/test-lint-rules/bpmnlint-plugin-custom/rules/awesome-send-task.js#L24

        name: node.name,
        meta: {
          type: "suggestion",
  
          docs: {
              description: "Enforce getter and setter pairs in objects and classes",
              recommended: false,
              url: "https://eslint.org/docs/latest/rules/accessor-pairs"
          }
        }
      });

I've included above meta data information. But the url didn't get added to the linting error message.

However I'm able to get this feature working following just by adding metadata to the rule and updating the linting module. Here's the proposed changes.

  1. Add documentation url to the rule metadata
    https://github.com/camunda/camunda-modeler/blob/develop/resources/plugins/test-lint-rules/bpmnlint-plugin-custom/rules/awesome-send-task.js#L24
reporter.report(node.id, 'This is awesome !!!😍', {
        name: node.name,
        documentationUrl: 'https://eslint.org/docs/latest/rules/accessor-pairs'
      });
  1. Read the documentation url in the linter.
    https://github.com/camunda/linting/blob/main/lib/Linter.js#L90
    const customDocumentationUrl = report.documentationUrl;
  2. Pass the customDocumentationUrl to getDocumentationUrl function.
    https://github.com/camunda/linting/blob/main/lib/Linter.js#L106
    url: getDocumentationUrl(rule, customDocumentationUrl)
  3. Update the getDocumentationUrl function indocumentation.js to read the customDocumentationUrl.
    https://github.com/camunda/linting/blob/main/lib/utils/documentation.js#L3
    export function getDocumentationUrl(rule, customDocumentationUrl = null) {
  4. Return the customDocumentationUrl.
    https://github.com/camunda/linting/blob/main/lib/utils/documentation.js#L32
    return customDocumentationUrl;

I've validated the changes in my local and I'm able to see the documentation url added to the linting error message.
As you suggested earlier this approach will help us define the URL yourself, right with the rule.
Please let me know if the changes looks good.

@nikku
Copy link
Member

nikku commented Sep 26, 2024

However I'm able to get this feature working following just by adding metadata to the rule and updating the linting module.

We want the meta-data to attach automatically, this likely requires a change in bpmnlint.

@kmannuru
Copy link
Contributor Author

However I'm able to get this feature working following just by adding metadata to the rule and updating the linting module.

We want the meta-data to attach automatically, this likely requires a change in bpmnlint.

Can you please help provide more insight regarding what we may need to update to attach the metadata automatically.Thanks.

@kmannuru
Copy link
Contributor Author

kmannuru commented Oct 23, 2024

@nikku I've created a draft PR with the changes. Can you please help review the changes.
I'm able to fix the behavior with below implementation.
camunda/linting#121

@nikku
Copy link
Member

nikku commented Jan 2, 2025

@kmannuru If you're still open to contribute this change, let's move along the desired solution sketch, camunda/linting#121 (comment).

@nikku
Copy link
Member

nikku commented Feb 17, 2025

Core support will ship via bpmn-io/bpmnlint#165.

@barmac
Copy link
Collaborator

barmac commented Feb 18, 2025

@nikku We can use #4829 together with the bpmnlint update.

@nikku nikku mentioned this issue Feb 20, 2025
4 tasks
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Feb 20, 2025
@bpmn-io-tasks bpmn-io-tasks bot removed the backlog Queued in backlog label Feb 20, 2025
@jarekdanielak jarekdanielak added this to the M86 milestone Feb 21, 2025
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BPMN enhancement New feature or request linting pr welcome We rely on a community contribution to improve this.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants