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 warning messages #377

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Fix warning messages #377

merged 3 commits into from
Feb 20, 2024

Conversation

pvandyken
Copy link
Contributor

All the warnings were displaying the wrong stack level: now corrected to show user code rather than snakebids code.

The warnings relating to bids were poorly worded or slightly inaccurate. These have been corrected

All the warnings were displaying the wrong stack level: now corrected to
show user code rather than snakebids code.

The warnings relating to bids were poorly worded or slightly inaccurate.
These have been corrected
@pvandyken pvandyken added the bug Something isn't working label Feb 19, 2024
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7558fc9) 91.61% compared to head (fe13caf) 91.62%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #377   +/-   ##
=======================================
  Coverage   91.61%   91.62%           
=======================================
  Files          39       39           
  Lines        1730     1731    +1     
=======================================
+ Hits         1585     1586    +1     
  Misses        145      145           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

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

Largely looks good to me - just wanted to clarify some wording in the warning message. It can get a little bit confusing when talking about both BIDS spec and snakebids` spec in the same message.

result = os.path.join(*path_parts) + tail
if custom_parts and _implicit and not in_interactive_session():
wrn_msg = (
f"Path generated with custom entities not part of the default BIDS "
Copy link
Contributor

Choose a reason for hiding this comment

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

The wording of this warning message is a little confusing to me. A few questions below that can hopefully clear some of this up:

  • What does a "default BIDS spec" mean here (latest)?
  • When talking about how "a spec has not been explictly declared", should this mention that we mean a snakebids' spec?
  • Related to the previous comment, clarification of "default spec" and "future specs" though I think these may have more context given the mention of upgrading snakebids to start the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

            wrn_msg = (
                f"Path generated with unrecognized entities, and a snakebids spec has"
                "not been explicitly declared. This could break in future snakebids "
                "versions, as the default spec can be changed without warning.\n"
                f"\tpath = {result!r}\n"
                f"\tentities = {custom_parts!r}\n\n"
                "Please declare a spec using:\n"
                "\tfrom snakebids import set_bids_spec\n"
                f'\tset_bids_spec("{specs.LATEST}")\n'
            ).expandtabs(4)

Less wordy. Hopefully "snakebids spec" is precise enough without needing a full definition, I think it's better to keep warning text brief. I could link the docs, but perhaps the code sample is enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this looks good to me. I tink the code sample is probably enough here, we could always look into linking the docs if necessary if users raise an issue about it.

@pvandyken
Copy link
Contributor Author

Also just fixed the error message for when multiple paths are found. That one's been bothering me for a while

@pvandyken pvandyken merged commit b9a5443 into khanlab:main Feb 20, 2024
36 checks passed
@pvandyken pvandyken deleted the fix/bids/warning branch February 20, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants