Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710503003:
Add Visual Studio Code documentation (Closed)
|
Created:
3 years, 10 months ago by ljusten (tachyonic) Modified:
3 years, 8 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Visual Studio Code documentation
Adds documentation about installation and usage of Visual
Studio Code as an editor for Chromium. The documentation
is to be referenced from the Chromium developers web page,
https://www.chromium.org/developers, as a sibling of Atom,
Sublime Text etc.
BUG=None
TEST=None
Review-Url: https://codereview.chromium.org/2710503003
Cr-Commit-Position: refs/heads/master@{#463964}
Committed: https://chromium.googlesource.com/chromium/src/+/e262c84162ef33baf5c484f3ab67d503c1c6c6a6
Patch Set 1 #Patch Set 2 : Added more documentation and polish #
Total comments: 21
Patch Set 3 : Merged with Jianpeng's version, cleaned up and extended a bit. #
Total comments: 4
Patch Set 4 : Renamed back. #Patch Set 5 : Comments + tweaks. #
Total comments: 13
Patch Set 6 : Tweaks. #
Total comments: 1
Messages
Total messages: 24 (7 generated)
ljusten@chromium.org changed reviewers: + pmarko@chromium.org
PTAL and thanks!
ljusten@chromium.org changed reviewers: + chaopeng@chromium.org
Hi Jianpeng, looks like you beat me to it! I'd like to suggest to merge our two documents into one if you're OK with that.
https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... File docs/visual_studio_code_dev.md (right): https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:1: # Visual Studio Code Dev Please merge this to docs/vscode.md https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:35: ## Updating This Page nit: Maybe we should not add this section. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:66: Add a line for developer easier install everything by 1 copy/paste. e.g. : `F1` - `ext install cpptools you-complete-me clang-format chromium-codesearch` https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:78: Wrap lines at 80 characters with `Alt+Q`. 1. Add chromium-codesearch https://marketplace.visualstudio.com/items?itemName=chaopeng.chromium-codesearch 2. Add clang-format, this extension can format c++/js. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:119: ctrl+` toggle terminal. F1 - CodeSearchOpen open current line in code search on chrome. F1 - CodeSearchReferences open XRef Infomation of current symbol. Use right click menu call go to definition or peek definition. Use right click menu call find all references. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:144: "editor.minimap.maxColumn": 80, "editor.minimap.renderCharacters": false This one feels better. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:169: Do you forget YCM settings here? Also please add how to install ycmd below. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:170: // C++ clang format settings. Please look at Nico's comment for clang-format https://codereview.chromium.org/2687733003/#msg11 https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:390: Please add my battery saving settings here: https://chromium.googlesource.com/chromium/src/+/master/docs/vscode.md#On-laptop https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:415: vscode can also be the difftool/mergetool. If you want to try, see https://github.com/Microsoft/vscode-tips-and-tricks/blob/master/README.md
PTAL. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... File docs/visual_studio_code_dev.md (right): https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:35: ## Updating This Page On 2017/03/30 14:16:14, chaopeng wrote: > nit: Maybe we should not add this section. I've added this because I imagine this doc will be read by many Nooglers who might not be familiar with the check-in process. It might literally be the first Chromium "code" they see, so I figured it would help to add this info here. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:66: On 2017/03/30 14:16:14, chaopeng wrote: > Add a line for developer easier install everything by 1 copy/paste. > e.g. : `F1` - `ext install cpptools you-complete-me clang-format > chromium-codesearch` Apparently they changed the behavior and ext install is not a command anymore. It opens the extension search window instead (also, it doesn't work with F1, but CTRL+P). Having all search results there is kind of confusing, try CTRL+P and ext install cpptools python chromium-codesearch toggle header/source proto you-complete-me rewrap It shows unrelated extensions mixed in, so I think it's better to have the user install them one by one. I've added chromium-codesearch, though, I didn't know that exists! https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:78: Wrap lines at 80 characters with `Alt+Q`. On 2017/03/30 14:16:14, chaopeng wrote: > 1. Add chromium-codesearch > https://marketplace.visualstudio.com/items?itemName=chaopeng.chromium-codesearch > > 2. Add clang-format, this extension can format c++/js. Done. Just noticed you wrote this extension. Nice work! Could you expose it to the Google-internal code search as well? It would be very useful for Chromium OS and other stuff that's only on internal code search. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:119: On 2017/03/30 14:16:14, chaopeng wrote: > ctrl+` toggle terminal. > F1 - CodeSearchOpen open current line in code search on chrome. > F1 - CodeSearchReferences open XRef Infomation of current symbol. > Use right click menu call go to definition or peek definition. > Use right click menu call find all references. Done. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:144: "editor.minimap.maxColumn": 80, On 2017/03/30 14:16:14, chaopeng wrote: > "editor.minimap.renderCharacters": false > This one feels better. I agree. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:169: On 2017/03/30 14:16:14, chaopeng wrote: > Do you forget YCM settings here? Also please add how to install ycmd below. Done. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:169: On 2017/03/30 14:16:14, chaopeng wrote: > Do you forget YCM settings here? Also please add how to install ycmd below. Added. How does use_imprecise_get_type work:true out for you? I'd rather wait longer and get precise type information. Is there a situation where it has a clear advantage? Btw, don't you also have to go into vim and install ycm there? https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:170: // C++ clang format settings. On 2017/03/30 14:16:14, chaopeng wrote: > Please look at Nico's comment for clang-format > https://codereview.chromium.org/2687733003/#msg11 Done. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:390: On 2017/03/30 14:16:14, chaopeng wrote: > Please add my battery saving settings here: > https://chromium.googlesource.com/chromium/src/+/master/docs/vscode.md#On-laptop Done. https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:415: On 2017/03/30 14:16:14, chaopeng wrote: > vscode can also be the difftool/mergetool. If you want to try, see > https://github.com/Microsoft/vscode-tips-and-tricks/blob/master/README.md Done.
chaopeng@chromium.org changed reviewers: + thakis@chromium.org
lgtm with nits. PTAL thakis@ https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... File docs/visual_studio_code_dev.md (right): https://codereview.chromium.org/2710503003/diff/20001/docs/visual_studio_code... docs/visual_studio_code_dev.md:169: On 2017/03/31 14:02:31, ljusten wrote: > On 2017/03/30 14:16:14, chaopeng wrote: > > Do you forget YCM settings here? Also please add how to install ycmd below. > > Added. How does use_imprecise_get_type work:true out for you? I'd rather wait > longer and get precise type information. Is there a situation where it has a > clear advantage? > > Btw, don't you also have to go into vim and install ycm there? No, I dont install ycm in vim. https://codereview.chromium.org/2710503003/diff/40001/docs/visual_studio_code... File docs/visual_studio_code_dev.md (right): https://codereview.chromium.org/2710503003/diff/40001/docs/visual_studio_code... docs/visual_studio_code_dev.md:84: You forgot clang-format (https://marketplace.visualstudio.com/items?itemName=xaver.clang-format) here. It is useful for layout test formatting. https://codereview.chromium.org/2710503003/diff/40001/docs/visual_studio_code... docs/visual_studio_code_dev.md:449: Add `[core] editor = "code --wait"` to your `~/.gitignore` file in order to use .gitignore or .gitconfig
Why is the filename changing? Seems like a sideways change, and makes reading the diff harder.
On 2017/03/31 14:43:29, Nico (afk until Tue Apr 4) wrote: > Why is the filename changing? Seems like a sideways change, and makes reading > the diff harder. I wanted to make naming consistent with sublime, but it seems like sublime is the exception to naming. I changed it back.
PTAL. I've moved chromium-codesearch to the might-be-useful section and added a warning since I was experiencing some major issues with message spam, extensions stopping working and go to definition not working on new code not in CS yet (the default C/C++ implementation works even on new code). I think it would be a bad experience for new users if they run into those issues and don't know why. https://codereview.chromium.org/2710503003/diff/40001/docs/visual_studio_code... File docs/visual_studio_code_dev.md (right): https://codereview.chromium.org/2710503003/diff/40001/docs/visual_studio_code... docs/visual_studio_code_dev.md:84: On 2017/03/31 14:42:19, chaopeng wrote: > You forgot clang-format > (https://marketplace.visualstudio.com/items?itemName=xaver.clang-format) here. > It is useful for layout test formatting. Since C/C++ already supports clang-format-on-save, which is sufficient at least for me, I added this to the section below. https://codereview.chromium.org/2710503003/diff/40001/docs/visual_studio_code... docs/visual_studio_code_dev.md:449: Add `[core] editor = "code --wait"` to your `~/.gitignore` file in order to use On 2017/03/31 14:42:19, chaopeng wrote: > .gitignore or .gitconfig Thanks, .gitconfig.
lgtm if chaopeng likes this, but the RHS is more than twice as long as the LHS, and much of the new stuff seems general "how to do use visual studio code" stuff, and I'm not sure if this is the place for that – imho our docs should document the chromium-specific bits of the tool, and there are many general intros on the interwebs. But up to you and chaopeng in the end. https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md File docs/vscode.md (right): https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode8 docs/vscode.md:8: [here](https://code.visualstudio.com/docs). People who read this doc probably know this already? https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode44 docs/vscode.md:44: TBR=<foo>@chromium.org This is true for all .md files, no need to mention this
> lgtm if chaopeng likes this, but the RHS is more than twice as long as the LHS, > and much of the new stuff seems general "how to do use visual studio code" > stuff, and I'm not sure if this is the place for that – imho our docs should > document the chromium-specific bits of the tool, and there are many general > intros on the interwebs. But up to you and chaopeng in the end. Thanks for your input. I had a slightly other approach to these docs, namely that of a Noogler who wants to pick an IDE that works well for Chromium. I'll discuss this with Pavol a bit, who also used VS Code as a Noogler. The doc was written independently of chaopeng's initially. I only found out about the existing one later. I've talked to him and he was fine with merging the two versions. https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md File docs/vscode.md (right): https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode8 docs/vscode.md:8: [here](https://code.visualstudio.com/docs). On 2017/04/06 14:23:23, Nico wrote: > People who read this doc probably know this already? This is written for Nooglers who read through https://www.chromium.org/developers and want to pick an IDE that works best for them in Chromium, so having this info seems valuable. I went through the setup fairly recently and that was my thinking. The section not for people who already know VS Code and want to know how to set it up for Chromium. https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode44 docs/vscode.md:44: TBR=<foo>@chromium.org On 2017/04/06 14:23:23, Nico wrote: > This is true for all .md files, no need to mention this This targets Nooglers specifically as well. A Noogler probably hasn't written a Chromium CL yet and doesn't know these details. I want to lower the bar for Nooglers to go in and update the document if they find errors because this is much more cumbersome to update than a Wiki page for a new developer.
https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md File docs/vscode.md (right): https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode44 docs/vscode.md:44: TBR=<foo>@chromium.org On 2017/04/10 12:07:58, ljusten wrote: > On 2017/04/06 14:23:23, Nico wrote: > > This is true for all .md files, no need to mention this > > This targets Nooglers specifically as well. A Noogler probably hasn't written a > Chromium CL yet and doesn't know these details. I want to lower the bar for > Nooglers to go in and update the document if they find errors because this is > much more cumbersome to update than a Wiki page for a new developer. I am OK with this part.
lgtm with nits (but I am not an OWNER) https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md File docs/vscode.md (right): https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode8 docs/vscode.md:8: [here](https://code.visualstudio.com/docs). On 2017/04/10 12:07:58, ljusten wrote: > On 2017/04/06 14:23:23, Nico wrote: > > People who read this doc probably know this already? > > This is written for Nooglers who read through > https://www.chromium.org/developers and want to pick an IDE that works best for > them in Chromium, so having this info seems valuable. I went through the setup > fairly recently and that was my thinking. > > The section not for people who already know VS Code and want to know how to set > it up for Chromium. > I agree that especially for a tool/IDE which is new such as vscode, it is a good idea to have this general overview section so people can read what it focuses on and decide if it's worth trying out. https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode44 docs/vscode.md:44: TBR=<foo>@chromium.org On 2017/04/10 14:24:54, chaopeng wrote: > On 2017/04/10 12:07:58, ljusten wrote: > > On 2017/04/06 14:23:23, Nico wrote: > > > This is true for all .md files, no need to mention this > > > > This targets Nooglers specifically as well. A Noogler probably hasn't written > a > > Chromium CL yet and doesn't know these details. I want to lower the bar for > > Nooglers to go in and update the document if they find errors because this is > > much more cumbersome to update than a Wiki page for a new developer. > > I am OK with this part. Not sure if the TBR part should be explicitly mentioned :) If TBRs are a good idea, it sounds as if we should change the folder structure / OWNERs? https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode59 docs/vscode.md:59: does store workspace settings in a .vscode folder in your base directory. You might want to clarify that the "base directory" is the argument given to vscode at startup (or pwd if omitted, I guess?). https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode69 docs/vscode.md:69: Complete C/C++ language support. nit: What does complete mean in this context?
(As I said, up to y'all in the end. I don't think it's generally true that you can equate "noogler" with "hasn't settled on an editor yet.) https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md File docs/vscode.md (right): https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode44 docs/vscode.md:44: TBR=<foo>@chromium.org On 2017/04/10 12:07:58, ljusten wrote: > On 2017/04/06 14:23:23, Nico wrote: > > This is true for all .md files, no need to mention this > > This targets Nooglers specifically as well. A Noogler probably hasn't written a > Chromium CL yet and doesn't know these details. I want to lower the bar for > Nooglers to go in and update the document if they find errors because this is > much more cumbersome to update than a Wiki page for a new developer. But following this argument, we'd add this note to every .md file, no?
>> I don't think it's generally true that you can equate "noogler" with "hasn't settled on an editor yet.) I didn't want to imply that all Nooglers are like that. There's certainly a good number that have been using other companies's editors before that only run on other companies's operating systems, though, but not on Linux;-) https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md File docs/vscode.md (right): https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode44 docs/vscode.md:44: TBR=<foo>@chromium.org On 2017/04/10 15:33:09, Nico wrote: > On 2017/04/10 12:07:58, ljusten wrote: > > On 2017/04/06 14:23:23, Nico wrote: > > > This is true for all .md files, no need to mention this > > > > This targets Nooglers specifically as well. A Noogler probably hasn't written > a > > Chromium CL yet and doesn't know these details. I want to lower the bar for > > Nooglers to go in and update the document if they find errors because this is > > much more cumbersome to update than a Wiki page for a new developer. > > But following this argument, we'd add this note to every .md file, no? I've removed the TBR part and linked to the documentation guidelines instead, I agree it was a bit too specific. We don't need the note in every .md file, but it certainly doesn't hurt to drop a line or two in a couple of possible entry points. https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode59 docs/vscode.md:59: does store workspace settings in a .vscode folder in your base directory. On 2017/04/10 15:20:11, pmarko wrote: > You might want to clarify that the "base directory" is the argument given to > vscode at startup (or pwd if omitted, I guess?). Done. 'code' seems to open the most recent instance, not a new instance in pwd. https://codereview.chromium.org/2710503003/diff/80001/docs/vscode.md#newcode69 docs/vscode.md:69: Complete C/C++ language support. On 2017/04/10 15:20:11, pmarko wrote: > nit: What does complete mean in this context? Added more buzzwords.
https://codereview.chromium.org/2710503003/diff/100001/docs/vscode.md File docs/vscode.md (right): https://codereview.chromium.org/2710503003/diff/100001/docs/vscode.md#newcode100 docs/vscode.md:100: working), so use with care. I also found go to definition is annoying and useless. will remove this feature.
The CQ bit was checked by ljusten@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chaopeng@chromium.org, pmarko@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2710503003/#ps100001 (title: "Tweaks.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491982011139400,
"parent_rev": "1305003fecaca059c4e3a5678eb48f0a00a46317", "commit_rev":
"e262c84162ef33baf5c484f3ab67d503c1c6c6a6"}
Message was sent while issue was closed.
Description was changed from ========== Add Visual Studio Code documentation Adds documentation about installation and usage of Visual Studio Code as an editor for Chromium. The documentation is to be referenced from the Chromium developers web page, https://www.chromium.org/developers, as a sibling of Atom, Sublime Text etc. BUG=None TEST=None ========== to ========== Add Visual Studio Code documentation Adds documentation about installation and usage of Visual Studio Code as an editor for Chromium. The documentation is to be referenced from the Chromium developers web page, https://www.chromium.org/developers, as a sibling of Atom, Sublime Text etc. BUG=None TEST=None Review-Url: https://codereview.chromium.org/2710503003 Cr-Commit-Position: refs/heads/master@{#463964} Committed: https://chromium.googlesource.com/chromium/src/+/e262c84162ef33baf5c484f3ab67... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e262c84162ef33baf5c484f3ab67... |
