Skip to content

Simplify location URLs if start and end line are the same#2405

Merged
aeisenberg merged 3 commits intogithub:mainfrom
Marcono1234:simpler-location-url
Jun 2, 2023
Merged

Simplify location URLs if start and end line are the same#2405
aeisenberg merged 3 commits intogithub:mainfrom
Marcono1234:simpler-location-url

Conversation

@Marcono1234
Copy link
Copy Markdown
Contributor

@Marcono1234 Marcono1234 commented May 7, 2023

When the start and end line of a result are the same, currently the extension creates location URLs which specify both line numbers, e.g. ...MyFile.js#L5-L5. This is appears to be redundant and can be simplified to just #L5.

I am not very familiar with this project, so any feedback is appreciated!

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@Marcono1234 Marcono1234 requested a review from a team as a code owner May 7, 2023 21:04
@aeisenberg
Copy link
Copy Markdown
Contributor

This looks great. Thank you for your contribution. One minor thing I would like to see is that with this change is that the test is no longer showing any multiline results (arguably this was a limitation of the test before, but there was only a single code path earlier). But, could you add at least one multiline result in one of the test files, like result-1-github-codeql.md? You'd also need to update the corresponding test input in analyses-results.json. I think this is straight forward, but if not, I can take a look.

@aeisenberg
Copy link
Copy Markdown
Contributor

Failing test is likely a flake.

@Marcono1234
Copy link
Copy Markdown
Contributor Author

Marcono1234 commented May 13, 2023

One minor thing I would like to see is that with this change is that the test is no longer showing any multiline results

I am not completely sure what you mean. If you had written "still shows multiline results", then I would have assumed you mean that something like #L1-L5 is unaffected, and that actually appears to be the case, see for example:

5. [npm-packages/meteor-installer/install.js](https://github.com/meteor/meteor/blob/73b538fe201cbfe89dd0c709689023f9b3eab1ec/npm-packages/meteor-installer/install.js#L11-L23)

[javascript/extractor/tests/regexp/input/multipart.js](https://github.com/github/codeql/blob/d094bbc06d063d0da8d0303676943c345e61de53/javascript/extractor/tests/regexp/input/multipart.js#L17-L20)

So, what did you mean by "no longer showing any multiline results"?

@aeisenberg
Copy link
Copy Markdown
Contributor

Apologies for that. You are correct. I missed those extra tests. Thanks for pointing them out. This PR looks great.

@aeisenberg aeisenberg enabled auto-merge June 2, 2023 17:47
@aeisenberg aeisenberg merged commit 610a349 into github:main Jun 2, 2023
@Marcono1234 Marcono1234 deleted the simpler-location-url branch June 3, 2023 16:50
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.

2 participants