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

Add enable-all-extensions option #5315

Merged
merged 2 commits into from
Nov 15, 2021

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Nov 15, 2021

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature

Description

@Pierre-Sassoulas 👀

Might as well put it in 2.12.

Partial of #5306.

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component Command line Related to command line interface labels Nov 15, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Nov 15, 2021
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM !

option = optikcontainer.add_option(*args, **optdict)
try:
option = optikcontainer.add_option(*args, **optdict)
except optparse.OptionConflictError:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Pierre-Sassoulas Are we sure this doesn't create problems?

Without it for some reason we get an error on accept-no-param-docs being a duplicate error. I couldn't figure out why it did this. This solves the issue and passes the tests, but I wonder if this creates other issues..

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it's strange, the error that is caught itself did not seem out of place for me, but if it's accept-no-param-docs being a duplicate error then it's an internal pylint error not an optparse error ? We should probably investigate if that's the case

Copy link
Collaborator Author

@DanielNoord DanielNoord Nov 15, 2021

Choose a reason for hiding this comment

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

I found the issue.
It is the extension check_docs. Because of it we are double loading docparams.

It has been deprecated for 5+ years. Perhaps it is time to remove it?

@coveralls
Copy link

coveralls commented Nov 15, 2021

Pull Request Test Coverage Report for Build 1464306898

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 93.436%

Totals Coverage Status
Change from base Build 1462545998: 0.04%
Covered Lines: 13908
Relevant Lines: 14885

💛 - Coveralls

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Sorry I just saw the coverage and realized it's not tested at all. Maybe it's doable in a functional test ?

@@ -489,7 +489,13 @@ def cb_verbose_mode(self, *args, **kwargs):
def cb_enable_all_extensions(self, option_name: str, value: None) -> None:
"""Callback to load and enable all available extensions"""
for filename in os.listdir(os.path.dirname(extensions.__file__)):
if filename.endswith(".py") and not filename.startswith("_"):
# pylint: disable=fixme
# TODO: Remove the check for deprecated check_docs after the extension has been removed
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall we remove the extension in 2.13? Or do we consider this 3.0 worthy?

Copy link
Member

Choose a reason for hiding this comment

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

Let's decide later. these todo list issue are very convenient, I'm going to do more of those. #5317 Feel free to edit :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You might want to look into Github projects, I find them even more convenient and much harder to lose out of sight as the issues on the front page increase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command line Related to command line interface Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants