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

Bug fixes #113

Merged
merged 8 commits into from
Jan 5, 2021
Merged

Bug fixes #113

merged 8 commits into from
Jan 5, 2021

Conversation

RobH123
Copy link
Contributor

@RobH123 RobH123 commented Jan 4, 2021

Closing manifest file errors


This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #113 (81f716c) into master (ff13706) will decrease coverage by 0.75%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
- Coverage   47.05%   46.29%   -0.76%     
==========================================
  Files          22       22              
  Lines        2782     2782              
  Branches      892      892              
==========================================
- Hits         1309     1288      -21     
- Misses        925      942      +17     
- Partials      548      552       +4     
Impacted Files Coverage Δ
src/core/manifest-text-check.js 4.81% <100.00%> (ø)
src/core/getApi.js 10.71% <0.00%> (-12.50%) ⬇️

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 ff13706...81f716c. Read the comment docs.

@RobH123 RobH123 linked an issue Jan 4, 2021 that may be closed by this pull request
@RobH123 RobH123 marked this pull request as draft January 4, 2021 08:50
@mandolyte
Copy link
Contributor

Since these functions use a straight parameter list, username is not optional. If this is a breaking change, you should make this part of a 1.0.0 (or 1.0.0-alpha1) release.

@RobH123 RobH123 marked this pull request as ready for review January 5, 2021 00:52
@ancientTexts-net
Copy link
Contributor


src/demos/file-check/checkFileContents.js, line 74 at r2 (raw file):

    checkFileResult = checkPlainText('text', filename, fileContent, ourCFLocation, checkingOptions);
  else if (filenameLower === 'manifest.yaml')
    // TODO: Does the branch need to be passed in as a parameter???

@RobH123 @mandolyte

Do we need to resolve this TODO before merging?
What do you think?

Thanks

Copy link
Contributor

@ancientTexts-net ancientTexts-net left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 7 files at r1, 6 of 7 files at r2.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @ancientTexts-net and @RobH123)

@ancientTexts-net
Copy link
Contributor


src/demos/notice-processing-functions.js, line 106 at r3 (raw file):

    // It only really makes sense if the debugChain is enabled
    if (givenNoticeObject.noticeList && givenNoticeObject.noticeList.length)
        if (givenNoticeObject.noticeList.length > 8000)

???

Copy link
Contributor

@ancientTexts-net ancientTexts-net left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RobH123)

@ancientTexts-net ancientTexts-net merged commit cfd0e49 into master Jan 5, 2021
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.

Fix manifest file errors in TN repo check
3 participants