v3.0.0 #217
v3.0.0 #217
Conversation
RobH123
changed the title
Codecov Report
@@ Coverage Diff @@
## master #217 +/- ##
===========================================
+ Coverage 29.88% 40.00% +10.12%
===========================================
Files 29 30 +1
Lines 4297 4504 +207
Branches 1632 1728 +96
===========================================
+ Hits 1284 1802 +518
+ Misses 1800 1643 -157
+ Partials 1213 1059 -154
Continue to review full report at Codecov.
|
|
Still more work for me to do of course @mandolyte, but I imagine you probably want to start using these fixes for OBS-TN/TQ/SN/SQ repos, plus a few other small improvements. |
There was a problem hiding this comment.
Reviewed 13 of 53 files at r1, 58 of 60 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RobH123)
src/core/wrapper.js, line 60 at r3 (raw file):
} return checkResults; }
Robert, the full set that tc-creates will support is (currently):
ta,
tw,
twl,
tn,
tq,
sq,
sn,
obs,
obs-tq,
obs-tn,
obs-sn,
obs-sq
Twelve in all. In the above there are not wrappers for any markdown format. Nor is any distinction made between Bible and OBS resource types. So two questions:
- Should there be wrappers for markdown resource types?
- Is there a need to distinguish between Bible and OBS for TQ, TN, SN, and SQ? (I see that Book Id might disambiguate).
Thanks!
|
@mandolyte Would you prefer, for example looking at 1/ Use the same wrapper function with bookID set to 'OBS' ? or It's just as easy either way for me. |
|
The first option sounds good, same function with bookid set to OBS. Thanks!
…On Thu, Oct 21, 2021, 5:10 PM Robert ***@***.***> wrote:
@mandolyte <https://github.com/mandolyte> Would you prefer, for example
looking at checkTWL_TSV6Table(username, languageCode, bookID, tableText,
checkingOptions) to have for OBS:
1/ Use the same wrapper function with bookID set to 'OBS' ? or
2/ Have a different wrapper function (without the bookID parameter):
checkOBSTWL_TSV6Table(username, languageCode, tableText, checkingOptions)`?
It's just as easy either way for me.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#217 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ2ZXPAR4K3KTLS3WFHHRDUIB6VHANCNFSM5FDJSTSQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@mandolyte Ok, I implemented #1 above. Please look at the wrapper here and see if it looks like it will meet your needs. (Since you don't seem to want to pass through the filename for the TN/TQ/SN/SQ/TW checks, you may not want to pass the articleFilepathInRepo parameter for TA/TW checks? Those parameters are mostly used to inform the user of the location of the error, but might also be used sometimes to check or ignore certain field/character combinations in certain places. So actually, thinking more about it, it probably would be better to pass in those filenames if they're easy available???) |
|
We have the bare file name available. Since the parameter name includes
"path" in the name, are you asking for the full path to file, with folders
and all?
…On Fri, Oct 22, 2021, 4:22 AM Robert ***@***.***> wrote:
@mandolyte <https://github.com/mandolyte> Ok, I implemented #1
<#1> above.
Please look at the wrapper here
<https://github.com/unfoldingWord/uw-content-validation/blob/new.2021.September.5/src/core/wrapper.js>
and see if it looks like it will meet your needs. (Since you don't seem to
want to pass through the filename for the TN/TQ/SN/SQ/TW checks, you may
not want to pass the articleFilepathInRepo parameter for TA/TW checks?
Those parameters are mostly used to inform the user of the location of the
error, but might also be used sometimes to check or ignore certain
field/character combinations in certain places. So actually, thinking more
about it, it probably would be better to pass in those filenames if they're
easy available???)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#217 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ2ZXJVX6KVGBRLVHBUF5DUIENNNANCNFSM5FDJSTSQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
@mandolyte The path within the repo. So for .tsv and manifest files, they're usually just filenames coz they're not in subfolders, but of course, in a markdown repo, a filename of Unless you object loudly, I think it makes sense to put that parameter back into all functions in the wrapper, even if you decide to only pass through an empty string. |
|
Agree. Thanks!
…On Fri, Oct 22, 2021, 3:41 PM Robert ***@***.***> wrote:
@mandolyte <https://github.com/mandolyte> The path within the repo. So
for .tsv and manifest files, they're usually just filenames coz they're not
in subfolders, but of course, in a markdown repo, a filename of 01.md is
not at all helpful -- something like content/kt/someword/01.md is
required to be of any use.
Unless you object loudly, I think it makes sense to put that parameter
back into all functions in the wrapper, even if you decide to only pass
through an empty string.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#217 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ2ZXKY2UUI52HWTBTJOEDUIG47XANCNFSM5FDJSTSQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Ok @mandolyte, I think this could be ready to go now -- see if those wrappers should do what you want. (I did add the filename/filepath fields back in, as they enable the package to suppress known non-errors for particular files.) |
|
@mandolyte We probably should see if this can be merged so we can get to v3. I'm not sure who's waiting on who here? (Meanwhile, I've moved on.) |
There was a problem hiding this comment.
Reviewed 9 of 14 files at r4, 25 of 25 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RobH123)
This change is