-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conflation of Pilot3 Issues #95, #135, #137, #138, and #139 per the discussion in the Issue #135 PR thread #140
Conflation of Pilot3 Issues #95, #135, #137, #138, and #139 per the discussion in the Issue #135 PR thread #140
Conversation
…sion thread in the Issue 135 PR. Applying some simple tidying to capture (i)efficiencies by linking the R Consortium WG Meeting Minutes starting with the October 2023 FDA Feedback with Planned Scheduling of the FDA Review Period and (ii)Pilot3 Team compilation of FDA Pilot3 Submission Feedback including the Pilot3 Team Issue Interpretation(s) as the FDA Review continues.
Hi @robertdevine - I was trying to fix the lintr issue when updating the pkg yml. I think the headline ("Feedback from the FDA") of the menu might need ot be updated if we are listing out the minutes for the meetings as well as specific feedback given to the FDA. Perhaps "Working Group Minutes and Regulatory Feedback" or "WG Minutes and Regulatory Feedback"? |
I managed to reduce the number of lintr issues. We can put in nolints to deal with the commented out code sections. Thanks @bms63, the update to the headline ("Feedback from the FDA") and "What's on the website section?" will be "WG Minutes and Regulatory Feedback". |
…latory Feedback, (ii)same for Pilot3 website What's on the Website? home page section, and (iii)tidy trailing spaces identified by lintr.
…rs around comments. GH lintr recommends removal of commented out code in the Details section of the GH Action. Pilot3 Team discussed the relevance of retaining the commented out code in the WG repository. Pilot3 Team Leads suggested using nolints for commented out code. Commented code lint disabled.
@bms63 and @laxamanaj - good to go on it. PR and Issue #140 pass all checks. This should address all of the discussion points following PR #135. Should be all set for the February 2024 WG General Meeting and further feedback from FDA. Thanks. again |
.lintr
Outdated
@@ -1,2 +1,2 @@ | |||
linters: linters_with_defaults(line_length_linter(150), object_usage_linter=NULL, cyclocomp_linter(complexity_limit = 20)) | |||
exclusions: list("R/data.R") | |||
linters: linters_with_defaults(line_length_linter(150), object_usage_linter=NULL, cyclocomp_linter(complexity_limit = 20), commented_code_linter = NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to just use nolint around the sections we don't want the lintr to check rather than disabling this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bms63, quite a few code blocks in Pilot3 with commented codes and 'unexpected chars' to apply nolints to address this item in a 'by section' manner where we don't want lintr to check. I'll chip away at it using the same branch and PR. thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ummm.. i wonder if there is a way to globally set to check after line 20 or something??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't see anything - lets just leave as it. I didn't realize we would need so many of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh way cool!! yeah I think we shouldn't have commented out code in our scripts, but since we have program headers we should tell the lintr not to check them . maybe set it to whatever the biggest header is?
README.md
Outdated
@@ -19,7 +19,7 @@ The website is divide into 4 sections that will walk you through our efforts: | |||
1. **Project Background/Setup** - Scope of work, what we used and how we used it. | |||
1. **Deliverables** - What was sent to the FDA - Cover Letter, TLFs, Datasets, ADRG. | |||
1. **Conferences** - Proceedings from different conferences on Pilot Submissions | |||
1. **FDA Feedback** - Specific feedback from FDA on this Submission | |||
1. **WG Minutes and Regulatory Feedback** - Specific feedback from FDA on this Submission |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we say something about there being two sections one on the minutes on the other on specific feedback from FDA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bms63, will do. Same branch and PR as well. The updated verbiage in the _What's on the Site? paragraph cites two sections under the WG Minutes and Regulatory Feedback menu, Section 1: Provides links to the monthly WG Meeting Minutes, Section2: Provides links to the Pilot3 Team Compilation of the Regulatory Review and the Pilot3 Team Interpretation(s) of the regulatory feedback as the Review Process continues. thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few thoughts - thanks for getting this going!!
…the Pilot3 website home page What's on the site? section WG Minutes and Regulatory Feedback menu noting two sections [links to monthly WG Meeting Minutes, link to Pilot3 Team Compilation and Interpretation(s) of the specific feedback from FDA as the Regulatory Review continues, and (iii)redacted commented code sections from the report-tlf-pilot3.Rmd source file. The redacted sections of commented out code should have little impact to reviewers.
Thanks @bms63 and @laxamanaj. Should be all set. Further tidying, (i)re-enabled the commented_code_linter, (ii)updated the Pilot3 website home page What's on the Website? paragraph WG Minutes and Regulatory Feedback menu noting two sections [links to monthly WG Meeting Minutes, link to Pilot3 Team Compilation and Interpretation(s) of the specific feedback from FDA as the Regulatory Review continues], and (iii)redacted commented code sections from the report-tlf-pilot3.Rmd source file. The redacted sections of commented out code should have little impact to reviewers. Thx. |
Per the Issue #135 PR discussion, tidying to capture (i)efficiencies by linking the R Consortium WG Meeting Minutes starting with the October 2023 FDA Feedback with Planned Scheduling of the FDA Review Period and (ii)Pilot3 Team compilation of FDA Pilot3 Submission Feedback including the Pilot3 Team Issue Interpretation(s) as the FDA Review continues.
Looks like lints fail during checks. I'll take a look. I think we saw this lints issue before. Thx.
Looks like a couple of stray chars, getting late, tomorrow I'll tidy a bit more and update the PR. Thx.
@bms63 and @laxamanaj - have it down to a single lintr error otherwise looks good - the Pilot3 URL in pkgdown yaml is not passing lint - we'll get it...I'll take a look.