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

fixed missing import of QTable #188

Merged
merged 10 commits into from
Apr 14, 2022

Conversation

kosack
Copy link
Contributor

@kosack kosack commented Apr 4, 2022

(also ran isort to optimize imports)

@kosack kosack requested a review from HealthyPear as a code owner April 4, 2022 15:15
@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #188 (a9f459d) into master (85155ea) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   63.06%   63.11%   +0.05%     
==========================================
  Files          28       28              
  Lines        2886     2890       +4     
==========================================
+ Hits         1820     1824       +4     
  Misses       1066     1066              
Impacted Files Coverage Δ
protopipe/benchmarks/operations.py 23.35% <100.00%> (+0.56%) ⬆️
protopipe/benchmarks/plot.py 14.10% <100.00%> (+0.55%) ⬆️
protopipe/benchmarks/utils.py 47.61% <100.00%> (+2.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85155ea...a9f459d. Read the comment docs.

Copy link
Member

@HealthyPear HealthyPear 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 you need to add QTable to the entries that automodapi should skip (protopipe/docs/benchmarks.rst)

@kosack
Copy link
Contributor Author

kosack commented Apr 4, 2022

Actually, that's not normally needed: it is however missing an __all__ entry, which is how sphinx determines what to document.

@HealthyPear
Copy link
Member

Actually, that's not normally needed: it is however missing an __all__ entry, which is how sphinx determines what to document.

Oh I didn't know. Then if that works it's fine for me.

@kosack
Copy link
Contributor Author

kosack commented Apr 4, 2022

Yeah, if a module doesn't have an __all__ entry, then everything in it becomes public, even imports. Which is why then Sphinx picks it up.

@HealthyPear
Copy link
Member

Yeah, if a module doesn't have an __all__ entry, then everything in it becomes public, even imports. Which is why then Sphinx picks it up.

I this is the case, I guess we can then also remove the :skip: directive(s) from the documentation file(s)

@HealthyPear
Copy link
Member

Yeah, if a module doesn't have an __all__ entry, then everything in it becomes public, even imports. Which is why then Sphinx picks it up.

I this is the case, I guess we can then also remove the :skip: directive(s) from the documentation file(s)

Actually we have to: the 2 things together generate warnings, which are set to be trated as errors by the documentation build on readthedocs (which for the moment is the only versioned one, even though #175 will add support for versioning also when building elsewhere)

@kosack kosack merged commit 926d68d into cta-observatory:master Apr 14, 2022
@kosack kosack deleted the fix/missing_import_plot_py branch April 14, 2022 13:52
@HealthyPear HealthyPear added the fix A PR that fixes a bug or a wrong behaviour. label Apr 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A PR that fixes a bug or a wrong behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants