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

adding check and fixing indexing error #91

Merged
merged 19 commits into from
Feb 3, 2023

Conversation

Melkiades
Copy link
Contributor

Fixes #90

@Melkiades Melkiades added bug Something isn't working sme Tracks changes for the sme board labels Jan 30, 2023
@Melkiades Melkiades marked this pull request as draft January 30, 2023 17:05
@Melkiades Melkiades marked this pull request as ready for review January 30, 2023 19:04
@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2023

badge

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------------------------
R/format_value.R       195      15  92.31%   72, 156, 175, 246, 359-366, 375, 386, 394
R/generics.R            82      20  75.61%   161-167, 180, 190, 243-248, 297-308, 381, 393, 426, 455, 467
R/labels.R              55       7  87.27%   53, 59, 68, 109, 137, 146, 150
R/matrix_form.R        174      34  80.46%   52-55, 185-200, 287, 309-314, 354-359, 381-391, 428
R/mpf_exporters.R      133     133  0.00%    3-221
R/page_size.R           43       8  81.40%   31-34, 64-70
R/pagination.R         184      44  76.09%   185, 199-208, 211-214, 221-240, 248-257, 287, 343-344, 346, 391
R/tostring.R           354      56  84.18%   7-12, 12-13, 13-14, 14-15, 15-16, 16-17, 17-18, 18-20, 20-22, 161-163, 205-206, 345-346, 430, 445, 552-566, 606, 642, 679-682, 686, 694, 735, 742
R/utils.R                1       0  100.00%
TOTAL                 1221     317  74.04%

Diff against main

Filename        Stmts    Miss  Cover
------------  -------  ------  -------
R/tostring.R      +15      +1  +0.40%
TOTAL             +15      +1  +0.24%

Results for commit: 5c5f382

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@gmbecker
Copy link
Collaborator

@cicdguy we are still getting the (quite disruptive) false positives fromt he SuperLinter for the formatters package.

@cicdguy
Copy link
Contributor

cicdguy commented Jan 31, 2023

@cicdguy we are still getting the (quite disruptive) false positives fromt he SuperLinter for the formatters package.

I believe cb2041e has resolved it. Are you talking about the annotations? If so, I can disable those also.

@Melkiades
Copy link
Contributor Author

@cicdguy we are still getting the (quite disruptive) false positives fromt he SuperLinter for the formatters package.

I believe cb2041e has resolved it. Are you talking about the annotations? If so, I can disable those also.

I think Gabe was referring to the error there was before. Thank you for fixing it ;)

@gmbecker
Copy link
Collaborator

gmbecker commented Feb 1, 2023

@cicdguy we are still getting the (quite disruptive) false positives fromt he SuperLinter for the formatters package.

I believe cb2041e has resolved it. Are you talking about the annotations? If so, I can disable those also.

@cicdguy Yes please do, they're very disruptive and completely useless when trying to read the changes in the face of the mass of false positives

@@ -212,7 +219,8 @@ MatrixPrintForm <- function(strings = NULL,
prov_footer = prov_footer,
col_gap = col_gap,
table_inset = as.integer(table_inset),
has_topleft = has_topleft
has_topleft = has_topleft,
indent_size = indent_size
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not understand if this is good to be added here... I do not know if this is followed by an actual indentation somewhere before. Afterwards, it is for sure reconstructed as such

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still it works well now, and for the reindentation I use mat$indent_size so I think it makes sense to have it in the object creation here

@Melkiades Melkiades requested a review from gmbecker February 2, 2023 09:46
Copy link
Contributor

@shajoezhu shajoezhu left a comment

Choose a reason for hiding this comment

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

lgtm! Thanks a lot @Melkiades ! tested downstream in Chevron, it is now working.

@shajoezhu
Copy link
Contributor

Hi @gmbecker , hope you are well. Can you accept the changes here. The problem is now resolved. and we have several PRs downstream in Chevron is blocked by this. Thanks a lot!

@shajoezhu shajoezhu disabled auto-merge February 3, 2023 03:23
@cicdguy cicdguy merged commit 614df97 into main Feb 3, 2023
@cicdguy cicdguy deleted the 90_fix_wrapping_header_indentation@main branch February 3, 2023 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sme Tracks changes for the sme board
Projects
None yet
4 participants