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.

Sign up for GitHub

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

Jump to bottom

Adds missing assertions and reorganizes them #696

Merged
merged 24 commits into from
Mar 1, 2024
Merged

Adds missing assertions and reorganizes them #696

merged 24 commits into from
Mar 1, 2024

Conversation

averissimo
Copy link
Contributor

Pull Request

Changes descriptions

@averissimo averissimo linked an issue Feb 27, 2024 that may be closed by this pull request
[Bug]: Some public tm_ module parameters are not checked and unordered #686
Open
gogonzo
gogonzo reviewed Feb 28, 2024
View reviewed changes
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

Buddy, you've forgotten tm_a_regression. Few minor comments beside this

R/tm_file_viewer.R Show resolved Hide resolved
R/tm_a_pca.R Outdated Show resolved Hide resolved
R/tm_g_bivariate.R Show resolved Hide resolved
tests/testthat/test-utils.R Outdated Show resolved Hide resolved
R/tm_missing_data.R Show resolved Hide resolved
@averissimo
chore: replace "Send ui args" with "Make UI args"
cdf949d
@averissimo
feat: reorder assertions and add necessary
928eca2
@averissimo
fix: remove duplicated assertion
48c96c3
@averissimo
tests: expand tests of assert_single_selection to include public API
b43aecf
@averissimo
tests: only use public functions
c5294bb
@averissimo
tests: add default spec failure test
edc1d27
@averissimo
Merge pre-release
4e3f7fc
@averissimo
Merge remote-tracking branch 'origin/pre-release@main' into 686-asser…
f164561 
…tions_process@pre-release@main

* origin/pre-release@main:
  removed @nord tags and added return from shared_params
  missing comma
  641 non exported functions (#680)
  pre-release vignettes review (#681)
  [skip roxygen] [skip vbump] Roxygen Man Pages Auto Update
  Standardise function titles similar to what we have in tmc (#691)
  [skip actions] Bump version to 0.2.16.9026
  `tm_g_regression` labels are no longer allowed out of bounds (#675)
  [skip actions] Bump version to 0.2.16.9025
  Remove the internal function `var_labels` in favour of `teal.data::col_labels` (#690)
  [skip actions] Bump version to 0.2.16.9024
  Fix the silent error in `tm_a_pca` (#688)
@averissimo
Copy link
Contributor Author

tm_a_regression was already done upstream on pre-release@main branch. Now merged here and updated with Send/Make UI args

@gogonzo gogonzo self-assigned this Feb 29, 2024
gogonzo
gogonzo approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

Assertions are fine. I would remove few tests to avoid opening a new pandora box discussion

tests/testthat/test-tm_g_bivariate.R Outdated Show resolved Hide resolved
tests/testthat/test-tm_g_bivariate.R Outdated Show resolved Hide resolved
tests/testthat/test-tm_g_bivariate.R Outdated Show resolved Hide resolved
@kartikeyakirar kartikeyakirar mentioned this pull request Mar 1, 2024
Merged
17 tasks
@averissimo @gogonzo
Apply suggestions from code review
da72fc7 
Co-authored-by: Dawid Kałędkowski <[email protected]>
Signed-off-by: André Veríssimo <[email protected]>
@averissimo averissimo merged commit 8151b06 into pre-release@main Mar 1, 2024
@averissimo averissimo deleted the 686-assertions_process@pre-release@main branch March 1, 2024 09:50
@m7pr
Copy link
Contributor

m7pr commented Mar 1, 2024

You are true legends guys. Great work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chlebowa chlebowa chlebowa left review comments

@gogonzo gogonzo gogonzo approved these changes

Assignees

@gogonzo gogonzo

Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Some public tm_ module parameters are not checked and unordered
4 participants
@averissimo @m7pr @gogonzo @chlebowa