Skip to content

R 1.0.0 #296

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 17 commits into from
Dec 8, 2017
Merged

R 1.0.0 #296

merged 17 commits into from
Dec 8, 2017

Conversation

lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Dec 5, 2017

Not to be merged. Aims to close #145.

@codecov-io
Copy link

codecov-io commented Dec 5, 2017

Codecov Report

Merging #296 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #296   +/-   ##
=======================================
  Coverage   91.47%   91.47%           
=======================================
  Files          30       30           
  Lines        1326     1326           
=======================================
  Hits         1213     1213           
  Misses        113      113
Impacted Files Coverage Δ
R/indent.R 95.38% <ø> (ø) ⬆️
R/ui.R 100% <ø> (ø) ⬆️
R/initialize.R 97.5% <ø> (ø) ⬆️
R/nested_to_tree.R 100% <100%> (ø) ⬆️
R/style_guides.R 98.13% <100%> (ø) ⬆️
R/serialized_tests.R 72.44% <100%> (ø) ⬆️
R/set-assert-args.R 79.31% <100%> (ø) ⬆️

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 cf08eec...285dba4. Read the comment docs.

@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Dec 5, 2017

This is the result from devtools::build_win(). Just this one note

checking CRAN incoming feasibility ... NOTE

https://win-builder.r-project.org/0uoMp9FDVJVz/00check.log

All the other things from #145 are incorporated, so I think we are ready for the submission. Do we need to state this other note separately, i.e. per built environment? Also why is there no note about the fact that this is a new release?

@lorenzwalthert
Copy link
Collaborator Author

Also, @krlmlr can you update the pkgdown page before the submission?

@lorenzwalthert
Copy link
Collaborator Author

For NEWS.md: Do we throw away all previous entries in the file so only the API description is left?

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

A few more minor nits, we might need to review initialize_attributes.

DESCRIPTION Outdated
@@ -1,6 +1,6 @@
Package: styler
Title: Non-invasive Pretty Printing of R code
Version: 0.0-11
Title: Non-Invasive Pretty Printing of R code
Copy link
Member

Choose a reason for hiding this comment

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

winbuilder complains that "code" needs to be capitalized too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right...

NEWS.md Outdated
- Create and serialize nested parse data.
- Internal support for indention of expressions with parentheses.
- Adding and removing spaces around operators on flat parse data.
## styler 0.1.0 (2017-12-05)
Copy link
Member

Choose a reason for hiding this comment

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

1.0.0 now, I forgot to update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

NEWS.md Outdated
style_text(text, ..., style = tidyverse_style, transformers = style(...))
```

### style guide creators
Copy link
Member

Choose a reason for hiding this comment

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

I think for a user the tidyverse_style() function is more important than create_style_guide().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, moved them up.

NEWS.md Outdated
This function is used to create a style guide.
```
create_style_guide(
initialize = initialize_attributes,
Copy link
Member

Choose a reason for hiding this comment

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

What is initialize_attributes? It should be either exported (and then perhaps be a function like default_style_guide_attributes()), or initialize should accept a default argument of NULL.

Copy link
Collaborator Author

@lorenzwalthert lorenzwalthert Dec 6, 2017

Choose a reason for hiding this comment

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

Yes, thanks, forgot about that. I think we may rename it to default_style_guide_attributes() then and export it.

Copy link
Member

Choose a reason for hiding this comment

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

Does this need an update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes -.-

NEWS.md Outdated

These are helper functions that simply forward to the helper functions above.
```
tidyverse_reindention()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add these two to the "style guides" section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok.

@krlmlr
Copy link
Member

krlmlr commented Dec 5, 2017

Yes, we can remove old news.

Added deploy key and restarted Travis job https://travis-ci.org/r-lib/styler/jobs/311941953, which should update pkgdown.

@krlmlr
Copy link
Member

krlmlr commented Dec 5, 2017

Restarted job https://travis-ci.org/r-lib/styler/jobs/311951016 instead, this is the correct 0.0-11 release.

@krlmlr
Copy link
Member

krlmlr commented Dec 6, 2017

Built pkgdown manually, for some reason the rebuild on Travis CI doesn't catch all changes.

@lorenzwalthert lorenzwalthert requested a review from krlmlr December 6, 2017 17:01
@lorenzwalthert
Copy link
Collaborator Author

Can't figure out why the appveyor buid fails now. Maybe currently updating to 3.4.3?

@lorenzwalthert lorenzwalthert removed the request for review from krlmlr December 6, 2017 17:07
@lorenzwalthert lorenzwalthert requested a review from krlmlr December 6, 2017 19:41
@lorenzwalthert
Copy link
Collaborator Author

New winbuild results
https://win-builder.r-project.org/81q44YxOBi0k/

checking CRAN incoming feasibility ... NOTE

Code is now capitalised in the DESCRIPTION, so everything should be fine now.

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Awesome, almost ready to go! I think we can ignore AppVeyor failures for now, krlmlr/r-appveyor#99.

NEWS.md Outdated
This function is used to create a style guide.
```
create_style_guide(
initialize = initialize_attributes,
Copy link
Member

Choose a reason for hiding this comment

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

Does this need an update?

@lorenzwalthert
Copy link
Collaborator Author

I also rephrased the help file for default_style_guide_attributes().

@lorenzwalthert
Copy link
Collaborator Author

Are we set for the release?

@lorenzwalthert lorenzwalthert requested a review from krlmlr December 7, 2017 21:03
@lorenzwalthert
Copy link
Collaborator Author

Should we also style the source code with styler prior to the CRAN submission?

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

I think so.

This function is used to create a style guide.
```
create_style_guide(
initialize = default_style_guide_attributes,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need parens here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No there must not be braces, see the example in the docs

set_line_break_before_curly_opening <- function(pd_flat) {
  op <- pd_flat$token %in% "'{'"
  pd_flat$lag_newlines[op] <- 1L
  pd_flat
}
set_line_break_before_curly_opening_style <- function() {
  create_style_guide(line_break = set_line_break_before_curly_opening)
}
style_text("a <- function(x) { ! x }", style = set_line_break_before_curly_opening_style)

I hope the following is clear enough:

#' @param line_break A list of transformer functions that manipulate line_break
#'   information.

The other arguments are described in a similar fashion.

Do you think it's clear enough or should we describe each of these arguments more explicitly, i.e. like this:

#' @param line_break A list of bare transformer function names that manipulate
#'   line_break information.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Makes sense.

@krlmlr
Copy link
Member

krlmlr commented Dec 7, 2017

I don't mind postponing styling styler until after the CRAN release, but we can do it if you like.

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.

CRAN-ready for 1.0.0
3 participants