clion: Improve documentation #9325
Conversation
|
+@amcastro-tri for feature review, please. \CC @sherm1 FYI |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Leh me give this a try this morning and I'll let you know.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, platform LGTM missing
doc/clion.rst, line 81 at r1 (raw file):
--------------------------- To use Bazel in CLion, you **must** install a plugin supplied by Google. To
is there a way to somehow highlight these "must"s in a very visible way? could we box somehow these sentences? I know I am the worst use case since I only skim through manuals without actually reading them. So for bad readers like me having colored boxes is really helpful.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, platform LGTM missing
doc/clion.rst, line 81 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
is there a way to somehow highlight these "must"s in a very visible way? could we box somehow these sentences? I know I am the worst use case since I only skim through manuals without actually reading them. So for bad readers like me having colored boxes is really helpful.
Every step in this setup portion of this document (first 109 lines) is important. If you skip steps, that's on you.
We could imagine rewriting setup steps to be more like a numbered list (so you'd just checklist yourself through it) so that its easier to follow, but I wasn't going to try do to that in this PR.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, platform LGTM missing
doc/clion.rst, line 76 at r1 (raw file):
**Note**: It is not necessary to import your project into a *new* CLion project. Overwriting the old project is appropriate (i.e., the directory likely located in ``~/ClionProjects/project-name``).
BTW I think the above line is out of date. CLion doesn't appear to use ClionProjects any more. Perhaps just delete the parenthetical here?
doc/clion.rst, line 81 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Every step in this setup portion of this document (first 109 lines) is important. If you skip steps, that's on you.
We could imagine rewriting setup steps to be more like a numbered list (so you'd just checklist yourself through it) so that its easier to follow, but I wasn't going to try do to that in this PR.
BTW Consider saying that explicitly at the start of this section, like
NOTE: EVERY STEP BELOW IS CRITICAL TO GET CLION WORKING PROPERLY.
Read carefully, and don't skip anything.
There have been a surprising number of otherwise-intelligent-seeming people skipping steps in these instructions, including me.
jwnimmer-tri
force-pushed
the
clion-doc
branch
from
0be1d70 to
c0ddca5
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, platform LGTM missing
doc/clion.rst, line 81 at r1 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW Consider saying that explicitly at the start of this section, like
NOTE: EVERY STEP BELOW IS CRITICAL TO GET CLION WORKING PROPERLY. Read carefully, and don't skip anything.There have been a surprising number of otherwise-intelligent-seeming people skipping steps in these instructions, including me.
Done. I added it to very top of document for now. I think reorganizing the setup instructions will help a lot, too -- for the future.
There was a problem hiding this comment.
Using Drake's bazel wrapper does solve the problems reported. Thanks @jwnimmer-tri!
Reviewable status: 3 unresolved discussions, platform LGTM missing
doc/clion.rst, line 11 at r1 (raw file):
First, install Bazel and build Drake with Bazel, following :ref:`the Drake Bazel instructions <bazel>`.
I think worth highlighting this one just in case, since at least I missed that the first time (I hadn't run prereqs, such an amateur).
doc/clion.rst, line 82 at r1 (raw file):
To use Bazel in CLion, you **must** install a plugin supplied by Google. To install the plugin, open ``File > Settings``, select ``Plugins``, and press the
I never opened CLion for this step before importing Drake. I directly went from the "Welcome to CLion" window clickin on the little gear box "Configure" and in the drop down menu I clicked on "Settings" (there is also a "Plugins" option with short cuts things a bit). Maybe worth saying that since for new installations people will probably do this instead?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: 3 unresolved discussions, platform LGTM missing
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, platform LGTM missing
- Require that bazel_wrapper is enabled; failure to so is now fatal in recent CLion, instead of merely a nuisance. - Document newest plug-in compatibility. - Highlight important points in text.
jwnimmer-tri
force-pushed
the
clion-doc
branch
from
c0ddca5 to
1ceedf6
Compare
There was a problem hiding this comment.
Also for the record... if any folks who use CLion daily would like to take over this document, I have no problem letting it go. I have been keeping it updated only so that Bazel upgrades aren't stalled. If there was a CLion daily user who wanted to always test newest Bazel and plug-ins, that would likely catch more errors than I do in my 10 minutes of testing.
Reviewable status: 1 unresolved discussion, platform LGTM missing
doc/clion.rst, line 11 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I think worth highlighting this one just in case, since at least I missed that the first time (I hadn't run prereqs, such an amateur).
Done.
doc/clion.rst, line 82 at r1 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I never opened CLion for this step before importing Drake. I directly went from the "Welcome to CLion" window clickin on the little gear box "Configure" and in the drop down menu I clicked on "Settings" (there is also a "Plugins" option with short cuts things a bit). Maybe worth saying that since for new installations people will probably do this instead?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all discussions resolved, platform LGTM missing
There was a problem hiding this comment.
We have a lot of daily CLion use in Dynamics and would be happy to test new combos of Bazel/CLion/plugin. We don't know the magic incantations for fixing problems therein, though.
Reviewable status: all discussions resolved, platform LGTM missing
There was a problem hiding this comment.
Reviewable status: all discussions resolved, platform LGTM missing
There was a problem hiding this comment.
+@SeanCurtis-TRI for platform
Reviewable status: all discussions resolved, platform LGTM missing
There was a problem hiding this comment.
QUick question on possible indentation aberration; otherwise
Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri]
doc/clion.rst, line 63 at r3 (raw file):
- CLion 2018.1.6 with: - Bazel plug-in 2018.08.06.0.1.
nit: misaligned indent?
There was a problem hiding this comment.
... magic incantations for fixing problems therein, though.
I'm still happy to work on fixes. Just the retesting every 3-4 weeks doesn't always fit into my schedule well (to do thoroughly).
Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri]
doc/clion.rst, line 63 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: misaligned indent?
I wanted to make it clear that this was a CLion-specific Bazel plug-in (not, say, a command-line plug-in to Bazel itself), so I tried to nest it here. Since the plug-in name is no longer "CLion with Bazel" it seemed kind of wish-washy without some affordance here. I am open to better ideas though (this one is admittedly subtle, though looks better rendered in HTML than monospace preview).
What do you think?
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, platform LGTM from [seancurtis-tri]
doc/clion.rst, line 63 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I wanted to make it clear that this was a CLion-specific Bazel plug-in (not, say, a command-line plug-in to Bazel itself), so I tried to nest it here. Since the plug-in name is no longer "CLion with Bazel" it seemed kind of wish-washy without some affordance here. I am open to better ideas though (this one is admittedly subtle, though looks better rendered in HTML than monospace preview).
What do you think?
I thought that might be the case (hence the question mark). To that end I have two thoughts:
- You could say "CLion Bazel plug-in ...."
- You could increase the indentation so that it's as much indented from the previous bullet as the bullet is from the margin.
Or both. That said, as long as this renders correctly, I'm satisfied -- the vast majority of people will experience this via the rendered webpage.
And for my own edification, what's the trick for rendering these files as part of the review process?
Leaving this up to you and marking myself as satisfied.
There was a problem hiding this comment.
Reviewable status:
doc/clion.rst, line 63 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I thought that might be the case (hence the question mark). To that end I have two thoughts:
- You could say "CLion Bazel plug-in ...."
- You could increase the indentation so that it's as much indented from the previous bullet as the bullet is from the margin.
Or both. That said, as long as this renders correctly, I'm satisfied -- the vast majority of people will experience this via the rendered webpage.
And for my own edification, what's the trick for rendering these files as part of the review process?
Leaving this up to you and marking myself as satisfied.
FYI Pull the PR locally, then:
bazel run doc:serve_sphinx
-- http://drake.mit.edu/documentation_instructions.html
|
Several good suggestions in the above will wait for the next PR. |
recent CLion, instead of merely a nuisance.
Closes #9282.
This change is