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

show test_that() calls in document outline #11108

Merged
merged 23 commits into from May 6, 2022
Merged

show test_that() calls in document outline #11108

merged 23 commits into from May 6, 2022

Conversation

romainfrancois
Copy link
Contributor

Intent

addresses #11082

Approach

Specializes TYPE_BRACE for test_that() calls.

Using emoji ✅ instead of having redundant test_that() everywhere.

image

But using test_that() in the thing below:

image

Automated Tests

Indicate whether this change includes unit tests or integration tests, or link a work item or pull request to add those tests to another repo. If the change cannot or will not be covered by a test, indicate why.

QA Notes

Add additional information for QA on how to validate the change, paying special attention to the level of risk, adjacent areas that could be affected by the change, and any important contextual information not present in the linked issues.

Checklist

  • If this PR adds a new feature, or fixes a bug in a previously released version, it includes an entry in NEWS.md
  • If this PR adds or changes UI, the updated UI meets accessibility standards
  • A reviewer is assigned to this PR (if unsure who to assign, check Area Owners list)
  • This PR passes all local unit tests

@romainfrancois
Copy link
Contributor Author

oh, this should not use the function icon here:

image

@romainfrancois
Copy link
Contributor Author

Should probably be a single test, i.e. an icon with a single ✅ instead of 3.

image

@romainfrancois
Copy link
Contributor Author

Separated this from #11102 because it's really two different things

@romainfrancois romainfrancois mentioned this pull request May 3, 2022
Merged
4 tasks
@romainfrancois
Copy link
Contributor Author

cc @hadley @maelle you might have views on how to display them on the outline and the status bar.

kevinushey
kevinushey reviewed May 4, 2022
View reviewed changes
src/gwt/acesupport/acemode/r_scope_tree.js Outdated
startPos,
ScopeNode.TYPE_BRACE,
{
"name": "✅ " + desc,
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should just use the name as-is, and consider an icon or styling (if any) on the GWT side.

(In addition, some fonts have trouble rendering certain UTF-8 symbols, so if we do want an icon or embellishment I think we should use an image)

It might also suffice to just use italics for the text label, or something similar, or leave the quotes in (since only test names would be quoted)

@maelle
Copy link

maelle commented May 5, 2022

So cool to see this in progress 😁 🙏
I'd prefer not to read too much italic but don't have any informed opinion. 😇

@romainfrancois romainfrancois force-pushed the feature/test_outline_navigation_11082 branch from 71ff5a1 to 7000105 Compare May 5, 2022 08:26
@romainfrancois
Copy link
Contributor Author

Maybe with some custom style, i.e. a colour:

image

here dark green, but could be inspired from test that ? e.g. some dark red ?

@romainfrancois
Copy link
Contributor Author

I think leaving the quotes is overwhelming, and an icon or emoji as before would be too much. We would rarely functions and tests anyway in a single test file. It's better practice to have helper functions in separate files.

cc @jennybc you might have views here.

@jennybc
Copy link
Member

jennybc commented May 5, 2022

This looks very nifty!

It's interesting that this intersects with the question of "what about code in the test file that's not inside test_that()?". As you say @romainfrancois, it's becoming more clear that that is often a smell. So it's interesting if the UI helps to discourage that practice.

What happens currently in this PR in the document outline for top-level code outside of test_that()? Just in general and for the specific case of a function definition.

@romainfrancois
Copy link
Contributor Author

What happens currently in this PR in the document outline for top-level code outside of test_that()? Just in general and for the specific case of a function definition.

Not much because the outline would only show functions and sections. but we could treat functions differently depending whether they belong or not.

image

but maybe it's more a job for source marker annotations ?

image

Or some more ambitious dedicated "Test" pane that would enforce good practice.

@jennybc
Copy link
Member

jennybc commented May 5, 2022

I think not appearing in the outline (non-function assignments) or appearing in a different colour (function definitions) is reasonable. It weakly discourages and signals that a bunch of test_that() calls is what one most expects to see in such a file.

@kevinushey
Copy link
Contributor

kevinushey commented May 5, 2022 •
edited

For existing prior art, if users have opted into showing both chunks and sections in the document outline:

Screen Shot 2022-05-05 at 12 27 08 PM

The display is like so:

Screen Shot 2022-05-05 at 12 27 19 PM

That is, we use italics for the chunk names, to visually distinguish them from other sections.

However, it's possible (as @maelle said) that italics could become overwhelming if every entry in the document outline were italicized, though. Can you share a screenshot with test labels using italics + no extra color, for comparison?

As an aside, if we choose a color for the text we need to make sure it remains visible in different editor themes (especially Solarized and dark themes).

We could also invert the relationship and choose to display R functions with italics for test files, and regular tests with "normal" styling.

(I guess my bias would be mildly towards "regular" styling, but choosing to italicize R function names within a test file)

@romainfrancois romainfrancois force-pushed the feature/test_outline_navigation_11082 branch from d660c29 to 684b489 Compare May 6, 2022 07:18
@romainfrancois
Copy link
Contributor Author

Going with altering opacity for secondary things (i.e. functions in test files). This should be more theme neutral, and nodes fade out rather than stand out (with italic):

For example, dplyr/test-mutate looks like this:

image

in solarized:

image

@maelle
Copy link

maelle commented May 6, 2022

is this enough contrast for all users?

@romainfrancois
Copy link
Contributor Author

Oops did not mean secondary to be italic.

image

image

It feels like a good compromise between having the item visible but more discrete. Happy to tweak the opacity up or down.

romainfrancois and others added 4 commits May 6, 2022 14:47
@romainfrancois
hyperlink get no default color (rstudio#11147)
c4641fb 
* update rstheme

* update expected rstheme
@romainfrancois
🇫🇷 for "plot" is "graphique" (rstudio#11148)
e85241c 
* update plot french text

* more

* tracé -> graphique

* plural
@romainfrancois
uiLanguage* properties in fr (rstudio#11119)
50af541 
* uiLanguage* properties in french

* add trainling : in fr and en
@jjallaire
visual editor: auto-prefix footnotes in included files
b5b242b 
support for 'editor' metadata options in files and projects
jjallaire and others added 2 commits May 6, 2022 14:35
@jjallaire
Merge branch 'main' of github.com:rstudio/rstudio into main
0826635
@romainfrancois
index test_that() calls for fuzzy finder (rstudio#11102)
1870ca0 
* index test_that calls for fuzzy finder

* use moveToNextSignificantToken()

* Don't unclude test_that() in the string

* rm quote from test desc

* make removeQuoteRegex static const

* testThatCAllIndexer adds "t " prefix

* only show test items if the search term starts with "t "

* fuzzy finder removes artifical "t " prefix

* remove dead code

* When search term starts with "t ", only include test items

* not looking for files and C++ items when only looking for test items

* NEWS

* factor out includeTestItems

* handle raw string in test_that(<raw string>)
@kevinushey
Copy link
Contributor

I like this as well. However, I agree we need to make sure the contrast remains high enough. If you're testing with a development build of RStudio Server + Chrome, you could use the built-in DevTools color contrast ratio checker:

https://juxtdesign.cc/guides/using-chrome-dev-tools-for-color-accessibility/

I think an opacity around 0.8 would probably be more appropriate.

kevinushey
kevinushey reviewed May 6, 2022
View reviewed changes
Copy link
Contributor

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

Code otherwise LGTM; I think we can merge once we've found an opacity that prefers contrast while de-emphasizing functions visually.

src/gwt/acesupport/acemode/r_scope_tree.js Show resolved Hide resolved
@romainfrancois
Copy link
Contributor Author

0.8:

image

0.7:

image

I feel like .7 is fine, I don't really see fading with .8 No strong feelings either way.

@romainfrancois
Copy link
Contributor Author

😬 I messed up with the merging, so I see a bunch of unrelated changes, but I suppose it should be fine if squashed.

@kevinushey
Copy link
Contributor

Thanks! 0.7 looks good to me there as well -- 0.8 doesn't look visually distinct enough.

@kevinushey kevinushey merged commit 25de4cb into rstudio:main May 6, 2022
@romainfrancois romainfrancois deleted the feature/test_outline_navigation_11082 branch May 6, 2022 20:10
@romainfrancois romainfrancois mentioned this pull request May 12, 2022
Closed
romainfrancois added a commit to romainfrancois/rstudio that referenced this pull request May 12, 2022
@romainfrancois
rm duplicate code, merge hiccup from rstudio#11108
207ba7a
romainfrancois added a commit to romainfrancois/rstudio that referenced this pull request May 13, 2022
@romainfrancois
rm duplicate code, merge hiccup from rstudio#11108
cd509d7
romainfrancois added a commit that referenced this pull request May 13, 2022
@romainfrancois
Outline uses quarto style labels, #| label: <name> (#11192)
7fbd9e9 
* rm duplicate code, merge hiccup from #11108

* look for quarto style labels, i.e. #| label: <name>

* Extract chunk labels from #| label: also in visual mode

* use \s instead of space

* update comment
@maelle maelle mentioned this pull request Jun 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinushey kevinushey kevinushey left review comments

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@romainfrancois @maelle @jennybc @kevinushey @jjallaire