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

New version with changed API for updated TSV formats #147

Merged
merged 16 commits into from
Mar 3, 2021

Conversation

RobH123
Copy link
Contributor

@RobH123 RobH123 commented Feb 11, 2021

This change is Reviewable

@RobH123 RobH123 marked this pull request as draft February 11, 2021 02:42
@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #147 (1da630b) into master (5410897) will increase coverage by 1.22%.
The diff coverage is 38.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   45.48%   46.71%   +1.22%     
==========================================
  Files          22       26       +4     
  Lines        3100     4044     +944     
  Branches     1006     1312     +306     
==========================================
+ Hits         1410     1889     +479     
- Misses       1063     1369     +306     
- Partials      627      786     +159     
Impacted Files Coverage Δ
src/core/disabled-notices.js 72.22% <ø> (ø)
src/core/field-link-check.js 0.00% <0.00%> (ø)
src/core/getApi.js 10.71% <0.00%> (ø)
src/core/usfm-js-check.js 11.76% <0.00%> (ø)
src/core/twl-tsv6-table-check.js 2.25% <2.25%> (ø)
src/core/twl-tsv6-row-check.js 2.52% <2.52%> (ø)
src/core/manifest-text-check.js 4.59% <6.66%> (-0.23%) ⬇️
src/core/yaml-text-check.js 1.36% <10.00%> (-0.04%) ⬇️
src/core/usfm-text-check.js 55.47% <27.48%> (-0.87%) ⬇️
src/core/notes-links-check.js 28.66% <28.66%> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5410897...1da630b. Read the comment docs.

@RobH123 RobH123 changed the title More improvements, esp. updated TSV formats New version with changed API for updated TSV formats Feb 11, 2021
@RobH123 RobH123 linked an issue Feb 12, 2021 that may be closed by this pull request
This was linked to issues Feb 18, 2021
@mandolyte
Copy link
Contributor

@RobH123 Could you connect this PR with an issue please? Our QA practices have standardized on this so that they and I as a reviewer have context and understanding of the changes.

I see this is still in draft mode, but I also wanted to ask whether this code would automatically detect whether a resource was in the new TSV format or not... in other words, will this be a breaking change that we have to coordinate deployment with the data format change.

Thanks!

@RobH123 RobH123 linked an issue Feb 25, 2021 that may be closed by this pull request
@RobH123
Copy link
Contributor Author

RobH123 commented Feb 25, 2021

Could you connect this PR with an issue please? Our QA practices have standardized on this so that they and I as a reviewer have context and understanding of the changes.

@mandolyte There were already six linked issues and I just added another one. I apologise from the ongoingness of this PR -- it's partly coz the new TSV formats were only finally approved yesterday, and partly because of how I work -- flitting back and forth from publishing/content work to software development and adding/fine-tuning checks as I understand the need for them during the publishing. Also partly coz I hadn't been able to devote a big-enough block of time to thorough testing of the new format checks.

@RobH123
Copy link
Contributor Author

RobH123 commented Feb 25, 2021

I see this is still in draft mode, but I also wanted to ask whether this code would automatically detect whether a resource was in the new TSV format or not... in other words, will this be a breaking change that we have to coordinate deployment with the data format change.

@mandolyte This big change is certainly a breaking change to the APIs (as reflected in the title and version number). That's partly why I wanted to also work on some issues/improvements that had been waiting for a break.

As to implementation of new formats in tC Create and your BP apps, I don't think that the roll-out has been fully planned yet. But here in general, each different TSV format has a different checking function. For the demos, I choose the formats here where TN2 and TQ2 and TWL will each cause the new checking functions to be called.

@RobH123 RobH123 linked an issue Mar 2, 2021 that may be closed by this pull request
@RobH123 RobH123 requested a review from mandolyte March 3, 2021 01:24
@RobH123
Copy link
Contributor Author

RobH123 commented Mar 3, 2021

I think this is finally working good enough to be a first attempt at v2

@RobH123 RobH123 marked this pull request as ready for review March 3, 2021 01:25
@RobH123 RobH123 linked an issue Mar 3, 2021 that may be closed by this pull request
@RobH123 RobH123 linked an issue Mar 3, 2021 that may be closed by this pull request
Copy link
Contributor

@mandolyte mandolyte left a comment

Choose a reason for hiding this comment

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

Reviewed 56 of 84 files at r2, 30 of 30 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mandolyte mandolyte merged commit 05e01f5 into master Mar 3, 2021
@mandolyte mandolyte deleted the RJHimprovements branch March 3, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment