Skip to content

Review: Alignment vignette #14

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

Merged
merged 4 commits into from
Oct 17, 2019

Conversation

pat-s
Copy link

@pat-s pat-s commented Oct 17, 2019

As per r-lib#258 (comment)

  • Put each sentence on a new line (makes reviewing text a lot easier on GH)
  • formatted pkg name as italics
  • broke up some long sentences into smaller ones

Regarding your thought of a too complex wording:
I guess this is at some point unavoidable here.
I would also not put too much energy into the vignette as the practical use case should be most descriptive and users who write aligned code usually do not really profit from it.

They will most likely try it out and see how styler does.


Maybe it would be of interest to add a R6 example using a dictionary structure (I couldn't find one right away in the mlr repos right now).

@lorenzwalthert
Copy link
Owner

lorenzwalthert commented Oct 17, 2019

Thanks. styler abides by the tidyverse style guide and hence uses 80 character width for all non-derivative files. Can you please re-format with stylermd?

# remotes::install_github("lorenzwlthert/stylermd")
stylermd::tidy_file("vignettes/detect-alignment.Rmd")

Now the diff is very hard for me to use the diff. Also, maybe it's worth adopting a different style, like the following use by Kirill Mueller: Start every new sentence on a new line. This way, you can have
80 character line width but if you change one sentence, it can't affect the diff of other sentences.

Reference: lorenzwalthert/stylermd#18

@pat-s
Copy link
Author

pat-s commented Oct 17, 2019

Sorry for the trouble. Applied the transformation. At least it was just one call :)

stylermd looks promising. A combined approach of "each sentence on a new line" and 80 chr line width would be great! I just do not see yet how that should work in practice: If you editor does a hard break at 80, there will often be a new line in most editors.
I think this can only work when combining "one sentence per line" and a 80 chr soft-wrap?

@lorenzwalthert
Copy link
Owner

Not sure. I will track this issue in lorenzwalthert/stylermd#18 if you are interested. I think the problem is that it will slow down styling because you first split by fullstop and then apply the styling to each sentence and then collapse back together.

@lorenzwalthert lorenzwalthert merged commit c43443e into lorenzwalthert:master Oct 17, 2019
@lorenzwalthert
Copy link
Owner

Thanks

@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #14 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage    90.8%   90.86%   +0.05%     
==========================================
  Files          43       43              
  Lines        1817     1817              
==========================================
+ Hits         1650     1651       +1     
+ Misses        167      166       -1
Impacted Files Coverage Δ
R/utils.R 83.33% <0%> (+5.55%) ⬆️

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 6d9e330...12aa820. Read the comment docs.

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