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

upgrade formatters and rlistings #298

Merged
merged 3 commits into from
Jan 14, 2025
Merged

upgrade formatters and rlistings #298

merged 3 commits into from
Jan 14, 2025

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Jan 14, 2025

There is also one more tests failing on dependency tests due to formatters+rlistings combination

 ── Error ('test-utils.R:25'): to_flextable: supported class `listing_df` ───────
  Error in `FUN(X[[i]], ...)`: argument "fontspec" is missing, with no default
  Backtrace:1. └─teal.reporter:::to_flextable(lsting) at test-utils.R:25:3
    2.   ├─rlistings::matrix_form(content)
    3.   └─rlistings::matrix_form(content)
    4.     └─rlistings (local) .local(obj, indent_rownames, expand_newlines)
    5.       ├─formatters::MatrixPrintForm(...)
    6.       │ └─base::structure(...)
    7.       ├─formatters::make_row_df(obj)
    8.       └─rlistings::make_row_df(obj)
    9.         └─rlistings (local) .local(...)
   10.           ├─rlistings:::vec_nlines(tt[[col]], max_width = colwidths[col])
   11.           └─rlistings:::vec_nlines(tt[[col]], max_width = colwidths[col])
   12.             ├─base::unlist(lapply(format_colvector(colvec = vec), nlines, max_width = max_width))
   13.             └─base::lapply(format_colvector(colvec = vec), nlines, max_width = max_width)
   14.               ├─formatters (local) FUN(X[[i]], ...)
   15.               └─formatters (local) FUN(X[[i]], ...)
   16.                 └─base::vapply(...)
   17.                   └─formatters (local) FUN(X[[i]], ...)
   18.                     └─formatters::wrap_txt(xi, max_width, fontspec = fontspec)
   19.                       └─formatters::open_font_dev(fontspec)

Signed-off-by: Marcin <[email protected]>
@m7pr m7pr added the core label Jan 14, 2025
@m7pr m7pr requested a review from llrs-roche January 14, 2025 09:45
Copy link
Contributor

github-actions bot commented Jan 14, 2025

Unit Tests Summary

  1 files   19 suites   28s ⏱️
197 tests 197 ✅ 0 💤 0 ❌
337 runs  337 ✅ 0 💤 0 ❌

Results for commit ead040f.

♻️ This comment has been updated with latest results.

@llrs-roche llrs-roche self-assigned this Jan 14, 2025
Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Do we really need 0.5.10? It seems formatters 0.5.8 already introduced the fontspec argument on wrap_txt that is needed according to the tests: https://diffify.com/R/formatters/0.5.5/0.5.8

@m7pr
Copy link
Contributor Author

m7pr commented Jan 14, 2025

@m7pr
Copy link
Contributor Author

m7pr commented Jan 14, 2025

Hey @llrs-roche I think we are dealing with an issue that is on an edge of rlistings and formatters. Both of them have been released recently and I think we need the newest versions of both to get this function to work. I updated the PR with a new version of rlistings as well and I am rerunning the job

@m7pr
Copy link
Contributor Author

m7pr commented Jan 14, 2025

@llrs-roche
Copy link
Contributor

llrs-roche commented Jan 14, 2025

I see that you added rlisting version too.
But the actions are still failing with:

Error in FUN(X[[i]], ...) : subscript out of bounds
Calls: fun ... new_release_deps_installation_proposal -> map_key_character -> vapply
Execution halted
Ran 1/1 deferred expressions

Triggering a new run

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

The new actions still fail with at ⠴ Resolving release version of withr [25/25] ETA: 0s

@m7pr
Copy link
Contributor Author

m7pr commented Jan 14, 2025

Yeah but it fails for release strategy and we accept this strategy might fail. This tests the package only with released dependencies, it does not use development dependencies. Sometimes package only works with the development dependencies and will no longer work with only released packages (that's why we need to keep releasing more often).

I was trying to fix the other strategy (min_isolated), which this PR actually fixed.

Would you mind accepting?

@m7pr m7pr changed the title upgrade formatters upgrade formatters and rlistings Jan 14, 2025
Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Oh, I didn't know that sometimes the release branch is allowed to break. All good then.

@m7pr
Copy link
Contributor Author

m7pr commented Jan 14, 2025

@llrs-roche so think about it like that

we have released packages:

teal.reporter ver 0.3.0 and it's dependency rtables 0.2.0

When rtables gets an improvement to the version 0.2.0.9001 but does not yet gets released, you sometimes need to satisfy new features in teal.reporter and you bump the dependency version to rtables (>= 0.2.0.9001). So now teal.reporter is dependent on the development version of the rtables. However release strategy only checks released packages, and the last released version is 0.2.0 for which teal.reporter breaks. So untill all dependencies are not released, the release strategy will keep breaking.

@m7pr m7pr merged commit 5148d25 into main Jan 14, 2025
35 of 36 checks passed
@m7pr m7pr deleted the fix_ci_formatters@main branch January 14, 2025 15:35
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants