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.

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

Documention with markdown and remove mentions to master branch #551

Merged
merged 24 commits into from
Aug 13, 2023

Conversation

olivroy
Copy link
Contributor

@olivroy olivroy commented Jun 15, 2023

Address #549

  • Redocument
  • Rename master -> main
  • Add @keywords internal to defunct functions
  • Convert to md documentation.
  • Remove @title tags, and some `@description tags
  • Reflow roxygen comments so the lines are not too wide.
  • Remove one instance of superseded tidyselect::one_of() with all_of().

Note: the style GHA doesn't work for me as I don't have write access. However, it fails. and recognizes that 10 files have to be changed.

@olivroy olivroy mentioned this pull request Jun 14, 2023
9 tasks
@olivroy olivroy changed the title Roxygen md Documention with markdown Jun 15, 2023
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #551 (cb5d203) into main (7375941) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #551   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         1183      1183           
=========================================
  Hits          1183      1183           
Files Changed Coverage Δ
R/adorn_ns.R 100.00% <ø> (ø)
R/adorn_pct_formatting.R 100.00% <ø> (ø)
R/adorn_percentages.R 100.00% <ø> (ø)
R/adorn_rounding.R 100.00% <ø> (ø)
R/adorn_title.R 100.00% <ø> (ø)
R/adorn_totals.R 100.00% <ø> (ø)
R/as_and_untabyl.R 100.00% <ø> (ø)
R/clean_names.R 100.00% <ø> (ø)
R/compare_df_cols.R 100.00% <ø> (ø)
R/excel_dates.R 100.00% <ø> (ø)
... and 12 more

@olivroy
Copy link
Contributor Author

olivroy commented Jun 16, 2023

For the review,
you may want to focus on 92450eb, 168c9de as those are the only manual changes I made except for conflict resolution)

These where I removed some @description tags (but kept others) I also use Reflow comments to make sure that the roxygen comments were not too wide.

I think the overall diff is also pretty good to look at!

Many commits are a bit useless as I was just trying to resolve conflicts manually or messed my local branches (62c1d04, 73a20bb, cc62e95, 81476f3)

The rest are automated changes initiated by styler, usethis, devtools or roxygen2md

@olivroy olivroy changed the title Documention with markdown Documention with markdown and remove mentions to master branch Jun 16, 2023
…index.

* However, the .Rd file is still created, so that people using `?convert_to_NA` will still see the documentation file.
@olivroy
Copy link
Contributor Author

olivroy commented Jun 16, 2023

I added a commit to add keywords internal to deprecated defunct functions. (Note that it was already there for crosstab() , I just added it to the other functions.

  • This will have the impact of removing those from the index file, hide them from the website's functions list, but still accessible from ?deprecated_functions

Copy link
Collaborator

@billdenney billdenney left a comment

Choose a reason for hiding this comment

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

This looks very helpful! I have a few minor comments on my read through.

R/make_clean_names.R Outdated Show resolved Hide resolved
R/remove_empties.R Outdated Show resolved Hide resolved
R/remove_empties.R Outdated Show resolved Hide resolved
R/round_half_up.R Show resolved Hide resolved
man/round_half_up.Rd Outdated Show resolved Hide resolved
@olivroy olivroy requested a review from billdenney June 16, 2023 15:37
@billdenney
Copy link
Collaborator

@sfirke , this looks good to me after the last couple of comments are resolved (which may be already done-- I'm reviewing from my phone).

@olivroy
Copy link
Contributor Author

olivroy commented Jun 16, 2023

@billdenney Thanks for the review. Your comments have been addressed. I added a commit last minute regarding @keywords internal. Details here #551 (comment)

I see you didn't have the change to review that, just wanted to flag that for transparency.

@billdenney
Copy link
Collaborator

I saw that one, and I appreciate the transparency. 😄

@olivroy
Copy link
Contributor Author

olivroy commented Aug 2, 2023

Hi @sfirke, friendly reminder!

whenever you have time, you can review this.

No user visible change overall.

Just some doc refactoring

Thanks

@sfirke sfirke self-assigned this Aug 11, 2023
Copy link
Owner

@sfirke sfirke left a comment

Choose a reason for hiding this comment

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

Looks great to me! This is a great effort. I left two small suggestions.

R/clean_names.R Outdated Show resolved Hide resolved
R/statistical_tests.R Show resolved Hide resolved
R/statistical_tests.R Outdated Show resolved Hide resolved
@sfirke
Copy link
Owner

sfirke commented Aug 11, 2023

hi @olivroy thanks for this nice cleanup job and sorry I took so long to review! A few comments above, the changes will be <5 minutes.

@olivroy
Copy link
Contributor Author

olivroy commented Aug 12, 2023

Thanks for your review!

Addressed in 24ef7bd

Minor additional tweaks pushed in 8418cb5

Styling in eb2c1ef

@sfirke sfirke merged commit b2700cf into sfirke:main Aug 13, 2023
9 checks passed
@sfirke
Copy link
Owner

sfirke commented Aug 13, 2023

Huzzah! Thanks again @olivroy 🙏

@olivroy olivroy deleted the roxygen-md branch August 13, 2023 13:06
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.

None yet

3 participants