Skip to content

fix#140: ordering of linting errors by source location#141

Open
RitoG09 wants to merge 4 commits into
sourcemeta:mainfrom
RitoG09:fix/140
Open

fix#140: ordering of linting errors by source location#141
RitoG09 wants to merge 4 commits into
sourcemeta:mainfrom
RitoG09:fix/140

Conversation

@RitoG09
Copy link
Copy Markdown
Contributor

@RitoG09 RitoG09 commented Dec 31, 2025

Fixes #140

What have been done

  1. The Sourcemeta Studio webview now renders lint diagnostics sorted by source location.
  2. e2e test cases written for linting order (lint-order.schema.json).

Proof of Action

image

@jviotti
Copy link
Copy Markdown
Member

jviotti commented Dec 31, 2025

The diff is huge, as it seems you are changing formatting/indention of the entire files you touch. Can you configure your editor to follow the current conventions so that the diff only reflects the actual changes?

The industry standard out there is EditorConfig (https://editorconfig.org), which you should be able to install on your editor. That will make it follow https://github.com/sourcemeta/studio/blob/main/.editorconfig

@jviotti
Copy link
Copy Markdown
Member

jviotti commented Dec 31, 2025

I'm also wondering if we can actually do the ordering on the CLI when lint --json is used? Then it will cascade here. And here we can keep the tests? If you are up to, feel free to open a PR for the CLI too

@RitoG09
Copy link
Copy Markdown
Contributor Author

RitoG09 commented Dec 31, 2025

I have formatted it using editorconfig in my latest commit, can u check it once?

@RitoG09
Copy link
Copy Markdown
Contributor Author

RitoG09 commented Jan 1, 2026

I'm also wondering if we can actually do the ordering on the CLI when lint --json is used? Then it will cascade here. And here we can keep the tests? If you are up to, feel free to open a PR for the CLI too

@jviotti will work on it and open a PR after 3-4 days as my semester end exams are going on.

@Karan-Palan Karan-Palan mentioned this pull request Jan 5, 2026
Merged
jviotti
jviotti reviewed Jan 5, 2026
View reviewed changes
Comment thread test/vscode/extension.test.ts Outdated
});

test("Lint diagnostics should be ordered by line number", async function () {
this.timeout(15000);
Copy link
Copy Markdown
Member

@jviotti jviotti Jan 5, 2026

Choose a reason for hiding this comment

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

Indentation problems here?

Copy link
Copy Markdown
Contributor Author

@RitoG09 RitoG09 Jan 5, 2026

Choose a reason for hiding this comment

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

fixed it.

jviotti
jviotti reviewed Jan 5, 2026
View reviewed changes
Comment thread vscode/src/extension.ts Outdated

if (lintResult.errors && lintResult.errors.length > 0) {
// TODO: Consider moving lint diagnostic ordering to the jsonschema CLI
lintResult.errors = sortLintErrorsByLocation(lintResult.errors);
Copy link
Copy Markdown
Member

@jviotti jviotti Jan 5, 2026

Choose a reason for hiding this comment

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

Indentation issues here?

Copy link
Copy Markdown
Contributor Author

@RitoG09 RitoG09 Jan 5, 2026

Choose a reason for hiding this comment

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

fixed it.

@RitoG09
Copy link
Copy Markdown
Contributor Author

RitoG09 commented Jan 5, 2026

@jviotti , the ordering is already implemented on CLI by @Karan-Palan . I think we should remove the todo comment.

@Karan-Palan
Copy link
Copy Markdown
Collaborator

Karan-Palan commented Jan 6, 2026

@jviotti , the ordering is already implemented on CLI by @Karan-Palan . I think we should remove the todo comment.

We might not need to merge this if ordering works fine on the CLI

@RitoG09
Copy link
Copy Markdown
Contributor Author

RitoG09 commented Jan 6, 2026

@jviotti , the ordering is already implemented on CLI by @Karan-Palan . I think we should remove the todo comment.

We might not need to merge this if ordering works fine on the CLI

yeah, it makes sense.

@jviotti
Copy link
Copy Markdown
Member

jviotti commented Jan 6, 2026

Let's merge the CLI PR first. I think we still want this PR, mostly because of the test.

@Karan-Palan
Copy link
Copy Markdown
Collaborator

Karan-Palan commented Jan 12, 2026

Hey @RitoG09, could you update the PR with keeping just the test and the related schema. Also, sign your commits to pass the DCO

@RitoG09
Copy link
Copy Markdown
Contributor Author

RitoG09 commented Jan 16, 2026

@Karan-Palan I'm traveling right now, will do this by Sunday.

@RitoG09 RitoG09 force-pushed the fix/140 branch 2 times, most recently from 33425e5 to 581bd73 Compare January 18, 2026 05:30
@RitoG09
fix#140: ordering of linting errors by source location
3031df9 
Signed-off-by: Ritabrata Ghosh <[email protected]>
@RitoG09
indentation fix
6ef0395 
Signed-off-by: Ritabrata Ghosh <[email protected]>
@RitoG09
extension level sorting omitted
0d408ff 
Signed-off-by: Ritabrata Ghosh <[email protected]>
@RitoG09
minor changes
3208757 
Signed-off-by: Ritabrata Ghosh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jviotti jviotti jviotti left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Order lint diagnostics by source location in the Studio webview

3 participants

@RitoG09 @jviotti @Karan-Palan