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

[FIX] force ordered output of _sanitize_reorder to match regex in test #3751

Merged
merged 4 commits into from Jun 13, 2023

Conversation

Remi-Gau
Copy link
Collaborator

Closes #3750

Changes proposed in this pull request:

  • force to print VALID_REORDER_VALUES in an ordered way

@Remi-Gau Remi-Gau changed the title [FIX] force ordered output of _sanitize_reorder to match regex in test [FIX] force ordered output of _sanitize_reorder to match regex in test Jun 13, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 13, 2023

👋 @Remi-Gau Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

  • PR has an interpretable title.
  • PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
  • Code is PEP8-compliant (see our documentation on coding style)
  • Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

  • There is at least one unit test per new function / class (see our documentation on testing)
  • The new feature is demoed in at least one relevant example.

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #3751 (91fc1d2) into main (7639219) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3751   +/-   ##
=======================================
  Coverage   91.60%   91.60%           
=======================================
  Files         139      139           
  Lines       16589    16594    +5     
  Branches     3242     3244    +2     
=======================================
+ Hits        15196    15201    +5     
  Misses       1390     1390           
  Partials        3        3           
Impacted Files Coverage Δ
nilearn/plotting/matrix_plotting.py 96.59% <100.00%> (+0.07%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Remi-Gau Remi-Gau marked this pull request as ready for review June 13, 2023 11:17
@@ -89,8 +89,7 @@ def test_sanitize_reorder(reorder):
def test_sanitize_reorder_error(reorder):
from ..matrix_plotting import _sanitize_reorder
with pytest.raises(ValueError,
match=("Parameter reorder needs to be "
f"one of {VALID_REORDER_VALUES}")):
match=("Parameter reorder needs to be ")):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My dirty fix is to make the regex less strict.

Copy link
Member

Choose a reason for hiding this comment

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

Yea I think that's ok. But do VALID_REORDER_ARGS and VALID_REORDER_VALUES need to be an unordered set? Can we just use a list instead?

Copy link
Collaborator Author

@Remi-Gau Remi-Gau Jun 13, 2023

Choose a reason for hiding this comment

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

I thought I had tried in the first commit but now realizing that the square brackets would turn add some "grouping" to the regex...

FAILED nilearn/plotting/tests/test_matrix_plotting.py::test_sanitize_reorder_error[None] - AssertionError: Regex pattern did not match.
 Regex: "Parameter reorder needs to be one of [False, True, 'average', 'single', 'complete']"
 Input: "Parameter reorder needs to be one of [False, True, 'average', 'single', 'complete']."

Copy link
Member

Choose a reason for hiding this comment

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

Ok right. you could maybe use re.escape("Parameter reorder needs to be one of [False, True, 'average', 'single', 'complete'].") but if you feel it's not worth the time we just go with the general pattern solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

decided to stringify everything

@Remi-Gau
Copy link
Collaborator Author

@ymzayek
quite unclear what kind of weird interaction led to such a specific failure, so for now I suggest making the the regex for match more general. LMKWYT

Copy link
Member

@ymzayek ymzayek left a comment

Choose a reason for hiding this comment

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

I think it's ok like this, thanks!

@Remi-Gau
Copy link
Collaborator Author

OK merging now since so we can avoid having this fail in other PRs

@Remi-Gau
Copy link
Collaborator Author

Thanks for the tips @ymzayek

@Remi-Gau Remi-Gau merged commit 2e9344d into nilearn:main Jun 13, 2023
29 checks passed
@Remi-Gau Remi-Gau deleted the fix-3750 branch June 13, 2023 13:37
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.

test error for order of printed argument with python 3.11
2 participants