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

Open
3 tasks
kmannuru opened this issue Aug 30, 2024 · 12 comments
Open
3 tasks

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

kmannuru opened this issue Aug 30, 2024 · 12 comments
Labels
backlog Queued in backlog BPMN enhancement New feature or request linting pr welcome We rely on a community contribution to improve this.

Comments

@kmannuru
Copy link

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
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
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
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
Contributor

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
Contributor

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
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:

@nikku
Copy link
Member

nikku commented Sep 19, 2024

Updated issue based on the confirmed solution sketch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Queued in backlog BPMN enhancement New feature or request linting pr welcome We rely on a community contribution to improve this.
Projects
None yet
Development

No branches or pull requests

3 participants