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.

Sign up for GitHub

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

Jump to bottom

Allow opening files at a specific line and column (fixes #5619) #5620

Merged
merged 2 commits into from
Oct 6, 2022
Merged

Allow opening files at a specific line and column (fixes #5619) #5620

merged 2 commits into from
Oct 6, 2022

Conversation

danog
Copy link
Contributor

@danog danog commented Oct 4, 2022

Fixes #5619

@danog danog requested a review from a team as a code owner October 4, 2022 18:28
@codecov
Copy link

codecov bot commented Oct 4, 2022 •
edited
Loading

Codecov Report

Merging #5620 (b45b356) into main (3a9eb31) will increase coverage by 0.08%.
The diff coverage is 66.66%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5620       /-   ##
==========================================
  Coverage   72.44%   72.52%    0.08%     
==========================================
  Files          30       30              
  Lines        1673     1678        5     
  Branches      366      366              
==========================================
  Hits         1212     1217        5     
  Misses        398      398              
  Partials       63       63
Impacted Files Coverage Δ
src/node/main.ts 50.00% <25.00%> (ø)
src/node/util.ts 91.80% <100.00%> ( 0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a9eb31...b45b356. Read the comment docs.

@jsjoeio
Copy link
Contributor

jsjoeio commented Oct 5, 2022

@danog do you plan to add tests? also, can you add to the PR description steps for testing this locally?

@danog
Copy link
Contributor Author

danog commented Oct 5, 2022 •
edited
Loading

Well it's pretty simple to test this locally, just open a workspace and run code-server -r path/to/file.ext:line:col from the workspace directory.
It works pretty much the same as before, the only difference is that now line and column numbers are supported.

@danog
Copy link
Contributor Author

danog commented Oct 5, 2022

Debug builds can be tested using node node_modules/ts-node/dist/bin.js out/node/entry.js -r path/to/file.ext:line:col

@jsjoeio
Copy link
Contributor

jsjoeio commented Oct 5, 2022

Thank you! I tested locally and it worked like a charm!

Screen.Recording.2022-10-05.at.12.39.19.PM.mov

Can you please add some tests?

jsjoeio
jsjoeio reviewed Oct 5, 2022
View reviewed changes
src/node/util.ts
Comment on lines 485 to 493
export const isDirectory = async (path: string): Promise<boolean> => {
try {
const stat = await fs.stat(path)
return stat.isDirectory()
} catch (error) {
return false
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test for this?

@jsjoeio
Copy link
Contributor

jsjoeio commented Oct 5, 2022

cc @code-asher - i tested locally but would love to get your thoughts too 👍🏼

code-asher
code-asher approved these changes Oct 5, 2022
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great. Glad it was so simple! Adding a simple test sounds like a good idea to me as well, can pretty much just copy the isFile one.

jsjoeio
jsjoeio suggested changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Add a test and then we should be good to merge!

@danog danog requested a review from jsjoeio October 6, 2022 17:15
jsjoeio
jsjoeio approved these changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Awesome! Looks good to me 👍🏼

@jsjoeio jsjoeio merged commit b562d4a into coder:main Oct 6, 2022
@jsjoeio
Copy link
Contributor

jsjoeio commented Oct 6, 2022

Thanks again for your contribution @danog! 🎉

@danog
Copy link
Contributor Author

danog commented Oct 11, 2022

Thanks for merging, could I have a tag please? :)

@jsjoeio
Copy link
Contributor

jsjoeio commented Oct 11, 2022

@danog by that do you mean tagged in the release notes? :D

@danog
Copy link
Contributor Author

danog commented Oct 12, 2022

I mean a new release, I've been thinking of proposing code-server as an alternative IDE at work, and our workflow heavily relies on the feature introduced in this PR :)

@jsjoeio
Copy link
Contributor

jsjoeio commented Oct 13, 2022

Oh no way, that's exciting! I'm going to see if I can finish #5624 and possibly do #5625 before the next release 🤞🏼

In the meantime, you should be able to see your changes by doing npm install -g code-server@beta (see here) if you use npm to install code-server.

@alanhe421
Copy link

Debug builds can be tested using node node_modules/ts-node/dist/bin.js out/node/entry.js -r path/to/file.ext:line:col

I think URL param-like r support is welcome.

@jsjoeio
Copy link
Contributor

jsjoeio commented Dec 5, 2022

We have an open issue for that: #1964

@code-asher code-asher mentioned this pull request Feb 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsjoeio jsjoeio jsjoeio approved these changes

@code-asher code-asher code-asher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feat]: --goto flag to open file @ line:column
4 participants
@danog @jsjoeio @alanhe421 @code-asher