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

Update the documentation url path to read from metadata #4829

Conversation

kmannuru
Copy link
Contributor

@kmannuru kmannuru commented Feb 12, 2025

Git issue: #4491

Related PRs:
Linting: Moved the logic to bpmnlint -- camunda/linting#129
bpmnlint: bpmn-io/bpmnlint#159

@barmac
Copy link
Collaborator

barmac commented Feb 13, 2025

Hi, as long as the metadata is not provided in the linting, we cannot really merge this. Or am I wrong?

@kmannuru
Copy link
Contributor Author

Hi, as long as the metadata is not provided in the linting, we cannot really merge this. Or am I wrong?

@barmac That's correct. I have made the changes to linting and bpmn-io-bpmnlint repos. I will share the dependent PRs shortly. Thanks.

@barmac barmac marked this pull request as draft February 14, 2025 12:57
@barmac
Copy link
Collaborator

barmac commented Feb 14, 2025

I'm converting this to draft then.

@kmannuru kmannuru marked this pull request as ready for review February 14, 2025 16:53
@kmannuru
Copy link
Contributor Author

@barmac The changes are ready for your review.

Here's the changes related to implementation:
Linting: Moved the logic to bpmnlint -- camunda/linting#129
bpmnlint: bpmn-io/bpmnlint#159

@barmac
Copy link
Collaborator

barmac commented Feb 18, 2025

Thanks, we will merge it together with the bpmnlint update.

@@ -91,7 +91,7 @@ function LintingTabItem(props) {

const {
category,
documentation = {},
meta = {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
meta = {},
meta,

Given the later line:

  const documentationUrl = meta?.documentation?.url;  

We get away with meta not existing.

@@ -55,8 +55,10 @@ describe('<LintingTab>', function() {
name: 'Foo',
message: 'Foo message',
rule: 'foo-rule',
documentation: {
url: 'https://foo.bar'
meta: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this requires adjustment in @camunda/linting to ensure that custom URLs are appropriately injected:

Copy link
Member

@nikku nikku Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, camunda/linting#129 should be the way to go. We can simply inject documentation in the original compat rules and this PR is fine without additional upstream adjustments. 👏

@nikku nikku added in progress Currently worked on needs review Review pending labels Feb 18, 2025 — with bpmn-io-tasks
@nikku nikku removed the in progress Currently worked on label Feb 19, 2025
@nikku
Copy link
Member

nikku commented Feb 20, 2025

Your PR will be incorporated with minor adjustments via #4843, thanks 👏.

Closing as superseded by #4843.

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.

3 participants