camunda / camunda-modeler

An integrated modeling solution for BPMN, DMN and Forms based on bpmn.io.
https://camunda.com/products/modeler
MIT License
1.51k stars 484 forks source link

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

Open kmannuru opened 2 months ago

kmannuru commented 2 months ago

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 https://github.com/camunda/camunda-modeler/issues/4491#issuecomment-2343756293 for discussion of different approaches.

camunda-modeler repo:

linting repo:

Additional context

No response

kmannuru commented 2 months ago

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

nikku commented 2 months ago

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

Let's consider some options:

kmannuru commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 :b: here)?

barmac commented 2 months ago

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 commented 2 months ago

@kmannuru given the feedback @nikku and I shared, I'd like to modify the issue to use the solution sketch from :b: in https://github.com/camunda/camunda-modeler/issues/4491#issuecomment-2343756293 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 commented 2 months ago

@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.

kmannuru commented 2 months ago

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 commented 2 months ago

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

nikku commented 2 months ago

Updated issue based on the confirmed solution sketch.

kmannuru commented 2 months ago

@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'
      });
  2. Read the documentation url in the linter. https://github.com/camunda/linting/blob/main/lib/Linter.js#L90 const customDocumentationUrl = report.documentationUrl;

  3. Pass the customDocumentationUrl to getDocumentationUrl function. https://github.com/camunda/linting/blob/main/lib/Linter.js#L106 url: getDocumentationUrl(rule, customDocumentationUrl)

  4. 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) {

  5. 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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. https://github.com/camunda/linting/pull/121