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

reconciling cran-fixes with main: autoplot.curve_params #254

Merged
merged 8 commits into from
Sep 23, 2024

Conversation

kristinawlai
Copy link
Collaborator

Review to remove conflicts between CRAN-fixes and main

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
R/autoplot.curve_params.R 0.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
R/autoplot.curve_params.R 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

@kristinawlai kristinawlai marked this pull request as draft September 20, 2024 03:14
@kristinawlai kristinawlai removed the request for review from d-morrison September 20, 2024 03:21
@kristinawlai kristinawlai marked this pull request as ready for review September 20, 2024 15:19
Copy link
Member

@d-morrison d-morrison left a comment

Choose a reason for hiding this comment

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

I'm approving this PR; see one minor comment below about line-breaking after each sentence. Also, the test coverage for this PR is too low (it's at 0%), which I usually wouldn't allow; I'm not sure why the system marked it as a success. I'm ignoring it this one time, since we are rushing to get you focused on the QE, but we need you to learn how to write unit-tests asap after that (and possibly sooner; I'll let you know if it becomes unavoidable).

#' Note that if you directly specify `rows_to_graph` when calling this function,
#' the row numbers are enumerated separately for each antigen isotype;
#' in other words, for the purposes of this argument,
#' row numbers start over at 1 for each antigen isotype. There is currently
Copy link
Member

Choose a reason for hiding this comment

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

In general, please line-break after every sentence; it makes code review easier later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@d-morrison question about this... in this case the sentences were too long to pass through linting (>80 characters), which is why I had to break them at weird points. What do you suggest for the future?

Copy link
Member

Choose a reason for hiding this comment

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

@kristinawlai ideally, add line breaks at all sentence ends and also at additional points to make all lines less than 80 characters; good places to break are after commas and before/after grammatical substructures (noun-phrases, verb-phrases, etc). I've recently started experimenting with turning my prose source code into an almost poetry-style format; the compiler will ignore single line breaks anyway and convert it back into a "normal" format, so you can have some fun with the source code formatting.

@kristinawlai
Copy link
Collaborator Author

Thanks @d-morrison! I'm also surprised that it passed checks without the unit tests... it did not pass them earlier and I was looking them up. Decided to submit it anyways since it passed though haha! Definitely on my radar.

@d-morrison d-morrison merged commit 82f1d11 into main Sep 23, 2024
13 checks passed
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.

2 participants