Skip to content

[BUG] Fix type error bug#130

Merged
RNKuhns merged 6 commits into
sktime:mainfrom
RNKuhns:fix_type_error_bug
Feb 23, 2023
Merged

[BUG] Fix type error bug#130
RNKuhns merged 6 commits into
sktime:mainfrom
RNKuhns:fix_type_error_bug

Conversation

@RNKuhns
Copy link
Copy Markdown
Contributor

@RNKuhns RNKuhns commented Feb 18, 2023

Reference Issues/PRs

Fixes #129.

What does this implement/fix? Explain your changes.

Error referenced in #129 was being raised through skbase.utils.check_sequence. This cleans up the error messaging there and in other type checking utility functions. They now raise TypeErrors and use a utility function to remove boilerplate "" when converting classes to strings.

Does your contribution introduce a new dependency? If yes, which one?

None.

What should a reviewer concentrate their feedback on?

Change in error messages.

Any other comments?

PR checklist

For all contributions
  • I've reviewed the project documentation on contributing
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
    the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added (see documentation standards)
  • New public functionality has been added to the API Reference

@RNKuhns RNKuhns requested a review from fkiraly February 18, 2023 18:03
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 18, 2023

Codecov Report

Merging #130 (35dcab8) into main (633b870) will increase coverage by 0.10%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   81.93%   82.03%   +0.10%     
==========================================
  Files          30       30              
  Lines        2236     2249      +13     
==========================================
+ Hits         1832     1845      +13     
  Misses        404      404              
Impacted Files Coverage Δ
skbase/utils/tests/test_iter.py 100.00% <ø> (ø)
skbase/utils/_iter.py 100.00% <100.00%> (ø)
skbase/validate/_types.py 100.00% <100.00%> (ø)
skbase/validate/tests/test_type_validations.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@RNKuhns RNKuhns mentioned this pull request Feb 18, 2023
@RNKuhns
Copy link
Copy Markdown
Contributor Author

RNKuhns commented Feb 18, 2023

@fkiraly this should be ready for review.

Copy link
Copy Markdown
Contributor

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I have a gut feeling that there might probably already be a method that does _remove_type_text, but I don't know, so fine with me.

My main design comment would be, test_type_validations, especially test_check_sequence_output looks like there is a lot of repetition that could be covered by pytest.mark.parametrize, but not a blocker for me.

@RNKuhns RNKuhns mentioned this pull request Feb 23, 2023
6 tasks
@RNKuhns RNKuhns merged commit 4de5c36 into sktime:main Feb 23, 2023
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.

[BUG] all_objects should return TypeError, not ValueError on wrong type

3 participants