-
Notifications
You must be signed in to change notification settings - Fork 7
Prepare for v3 #154
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
Open
yulric
wants to merge
23
commits into
dev
Choose a base branch
from
prepare-for-v3
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Prepare for v3 #154
+5,789
−3,828
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* The new columns are used to provide versioning information for each variable. * The version column is set to 2.2.0 which is the current version of the package * The lastUpdated column is set to the date in the v3.0.0 branch. The date does not really matter for this commit. * The status column is set to active.
The default value was bought over from the v3.0.0 branch
The columns now match up with what's in the v3.0.0 branch which should improve git diffs and make it easier to review changes
The column order now matches up with what's in the v3.0.0 branch which should improve diffs and make it easier to review changes.
9a82075 to
8e43bb5
Compare
This CEP proposes a standardization tool for variables.csv and variable_details.csv to ensure consistent formatting across different editors and operating systems, enabling clean semantic diffs in version control.
8e43bb5 to
9792e8a
Compare
…for formatting issues
…tests into its own file
…variable details sheet
…details sheets for formatting issues
When recoding derived variables the function was was using the wrong row to extract the name of the custom function name
…ions, `check_worksheet` and `fix_worksheet`
9792e8a to
642fc30
Compare
DougManuel
reviewed
Jan 2, 2026
Contributor
DougManuel
left a comment
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.
A few comments:
- Potentially missing columns - The YAML schemas in inst/metadata/schemas/core/variables.yaml include
notesin the expected order, and variable_details.yaml includestemplateVariableas an optional field.
Should these be added?
- If you remember, I drafted a similar validation for my work-in-progress. That is in feature/csv-standardisation-updates. Here is a comparison of features, in case you wanted to look that the validations that I included. The pattern, enum and cross-fields were errors that I was dealing with when reviewing and creating code.
| Feature | csv-standardisation-updates |
This PR |
|---|---|---|
| Column order validation | ✅ | ✅ |
| Row sorting validation | ❌ | ✅ |
| Line ending validation (LF/CRLF) | ✅ | ✅ |
| Excessive quoting detection | ❌ | ✅ |
| Trailing empty columns | ❌ | ✅ |
| Pattern validation (regex) | ✅ | ❌ |
| Enum validation | ✅ | ❌ |
| Cross-field validation | ✅ | ❌ |
| Auto-fix capability | ✅ | ✅ |
| GitHub Action | ❌ | ✅ |
- I validated using the rules in The YAML schemas in inst/metadata/schemas/core/variables.yaml for column names, ordering, etc. dynamically, rather than hardcoding. The "source of truth" is an interesting discussion.
I also drafted a more extensive report inspired by pkgdown and devtools. I like your error reporting very much, but take a look that that branch if you are interested.
Minor Items
- There's a capture.output(print(row_being_checked), file = "log.txt", append = TRUE) in recode-with-table.R:932 that looks like debug code - should that be removed?
- readr is used in fix_worksheet() but isn't in DESCRIPTION. I think the validation should be a pernament feature of cchsflow, so adding that to description is a consideration.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The purpose of this PR is to prepare the dev branch for merging the changes in from the v3.0.0 branch. To that end it does the following: