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] Styling issues and make RTD build work #968

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

eurunuela
Copy link
Collaborator

@eurunuela eurunuela commented Aug 9, 2023

Closes #967, Closes #961

Changes proposed in this pull request:

  • Uses type(a) is list instead of type(a) == list.
  • Increases the minimum version required for sphinx to 6.2.1 and sphinx_rtd_theme to 1.2.2, which should not use the removed mention of style.

@eurunuela eurunuela changed the title Fix styling issues [FIX] Styling issues Aug 9, 2023
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Patch coverage: 83.33% and project coverage change: -0.03% ⚠️

Comparison is base (966a262) 88.91% compared to head (18af6fb) 88.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #968      +/-   ##
==========================================
- Coverage   88.91%   88.88%   -0.03%     
==========================================
  Files          27       27              
  Lines        3375     3375              
  Branches      618      618              
==========================================
- Hits         3001     3000       -1     
  Misses        227      227              
- Partials      147      148       +1     
Files Changed Coverage Δ
tedana/workflows/ica_reclassify.py 97.79% <50.00%> (ø)
tedana/selection/selection_utils.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@handwerkerd
Copy link
Member

Both this PR and #966 look good to me, but they are separately failing checks (RTD build for this one and style for the RTD fix). Can you combine these changes into a single PR so that all the checks pass before merging? Otherwise I'm fine approving both and merging separately.

handwerkerd
handwerkerd previously approved these changes Aug 10, 2023
Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

LGTM. As I mentioned in a comment, I think it would be better to have one PR with this and #966 so that all checks pass, but I'll approve this in case we decide to merge separately.

@eurunuela
Copy link
Collaborator Author

eurunuela commented Aug 10, 2023

I can merge this with the other PR tomorrow. But I don't mind if they get merged separately either.

Edit: I will merge them together in the morning if they haven't been merged into main yet.

@eurunuela eurunuela changed the title [FIX] Styling issues [FIX] Styling issues and make RTD build work Aug 11, 2023
@eurunuela
Copy link
Collaborator Author

This PR is quite simple and is mostly testing-related. I think 1 approval would be enough.

Thoughts @handwerkerd?

@handwerkerd handwerkerd added bug issues describing a bug or error found in the project maintenance issues related to versioning, dependencies, and other related elements and removed bug issues describing a bug or error found in the project labels Aug 11, 2023
@tsalo tsalo mentioned this pull request Aug 11, 2023
Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

LGTM.
This is fairly minor and holding up a bunch of other PRs. I'll give @tsalo @n-reddy @marco7877 and @dowdlelt a few hours to see if they object and want to check this more carefully. Otherwise, I'll merge with just my approving review.

Copy link
Collaborator

@dowdlelt dowdlelt left a comment

Choose a reason for hiding this comment

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

Super simple, knock it out!

@handwerkerd handwerkerd merged commit 8daae00 into ME-ICA:main Aug 11, 2023
2 checks passed
@eurunuela eurunuela deleted the fix_linting branch September 12, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance issues related to versioning, dependencies, and other related elements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Style test is not passing RTD build is failing
3 participants