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

Include column numbers in location URLs #2406

Merged
merged 3 commits into from
Jul 19, 2023
Merged

Include column numbers in location URLs #2406

merged 3 commits into from
Jul 19, 2023

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented May 7, 2023

Include column numbers in the GitHub location URLs, in the format LstartlineCstartcolumnLendlineCendcolumn.

Warning: It looks like this is not fully supported by the GitHub web interface, neither for the old code view nor the new code view, see https://github.com/orgs/community/discussions/48301#discussioncomment-5831155. Might be good to clarify this with your colleagues.
No worries if you decide to reject this pull request.

Also, I am not very familiar with this project yet, so please let me know if overlooked something or if there is something else I should change. 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 review from a team as code owners May 7, 2023 21:35
// Old GitHub code view does not show any highlighting at all if start and end line differ; so for now
// only add column information when the line numbers are the same
if (startColumn && endColumn && startLine && startLine === endLine) {
// Note: Currently new GitHub code view ignores column information and highlights complete lines,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a link to the discussion where you mention this? In the future, this may be fixed and adding a link here would allow us to follow up as things change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what happens if you pass an end column number that is smaller than the start? Or what if you pass startColumn === endColumn && startLine === endLine?

This can be a user error if they define a bad source location in the query. It's probably harmless, and if so there's no need to handle or check for this. But, just would like to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, what happens if you pass an end column number that is smaller than the start? Or what if you pass startColumn === endColumn && startLine === endLine?

Good points! The old code view behaves like this:

@Marcono1234
Copy link
Contributor Author

Got a response on the discussion that this will be improved in the future.

Though maybe for now there is no point in including this in the extension yet if the old code view only supports some scenarios properly and the new code view ignores column information completely. I will therefore mark this pull request as Draft for now, I hope that is alright for you. Thank you nonetheless for your review already!

@Marcono1234 Marcono1234 marked this pull request as draft May 13, 2023 22:26
@Marcono1234
Copy link
Contributor Author

As small update: It looks like the new GitHub code view now highlights the sections correctly. However, because the new code view isn't the default yet and the old code view still does not support all cases this pull request will still have to wait.

@Marcono1234 Marcono1234 marked this pull request as ready for review July 15, 2023 20:05
@Marcono1234
Copy link
Contributor Author

Marcono1234 commented Jul 15, 2023

Have rebased this pull request onto the latest changes of main and also added checks to only include the column information in the URL if it is valid (i.e. either start-line < end-line or if on the same line start-column < end-column), as suggested by you.

The new code view is now also shown for anonymous users (see changelog entry). Therefore I have now marked this PR as ready.

However, there are still the following open issues:

  • If "Wrap lines" is enabled in the code view, the highlighted section might be wrong
  • If the highlighted section is too far on the right, the code view does not automatically scroll there, so the section might not be immediately visible

See https://github.com/orgs/community/discussions/48301#discussioncomment-6456645

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Thanks for tracking the deployment of the new code view and any bug fixes there. I think it makes sense to use this new feature now, but thanks for also highlighting a couple of edge cases where it's not perfect.

The extension changes look good to me. I've also tried running a variant analysis and the link generation all works correctly for me.

@Marcono1234
Copy link
Contributor Author

Marcono1234 commented Jul 18, 2023

Thanks! Should I edit CHANGELOG.md as well, or is this not considered 'user visible'?

Or of course feel free to edit the changelog yourself in case that is easier for you.

@robertbrignull
Copy link
Contributor

Good point. A changelog entry would be good for this since this would be considered a user-visible change.

I'll add a changelog entry and merge this PR 🚀

@robertbrignull robertbrignull merged commit f2c7c41 into github:main Jul 19, 2023
26 checks passed
@Marcono1234 Marcono1234 deleted the location-url-column branch July 19, 2023 20:30
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