Move argparse code into separate argparse modules

[shivam-Purohit]

Added module argparse to separate the functions and classes using the command-line argument parsing.
closes #367

[shivam-Purohit]

@spbnick Am I on the right path? is there something I am missing?
If not then I have to repeat this for all the other files like db, oo.

[spbnick]

Looks pretty good! Thank you, Shivam!

I only have a couple inline comments. Please proceed to the other modules after addressing them.

Please also observe commit guidelines in the PR I just posted.

Thank you 🙂

[shivam-Purohit]

I am sorry the commits are all over the place. It will be a hard task but could you review it please @spbnick? I am also checking if I missed anything.

[spbnick]

Nice work, @shivam-Purohit!

I have a few requests inline, and please don't hesitate to ask questions, if anything is not clear, or you need help ❤️

[spbnick]

Apart from the inline comments, please pay attention to the errors that the CI checks show, and the commit guidelines.

[shivam-Purohit]

action= PatternHelpAction,
should PatternHelpAction be also be moved to argparse?

[spbnick]

should PatternHelpAction be also be moved to argparse?

Yes, because it's closely related to argparse purpose.

[shivam-Purohit]

@spbnick I tried all related changes in one commit. I am getting flake8 and pylinter errors. I will fix all that once all the changes are done. I am also checking for any code errors I made. I have one suggestion if we could add pre-commit check feature for the commits? That will ensure a proper code even before committing.

[spbnick]

I have one suggestion if we could add pre-commit check feature for the commits? That will ensure a proper code even before committing.

Of course! I have pre-commit hook set up in my local repo. However, I'm not sure how to best distribute such a script. If you have an idea, please post a PR. You can take the commands to run from the GitHub test workflow, in the repo.

[shivam-Purohit]

If you have an idea, please post a PR. You can take the commands to run from the GitHub test workflow, in the repo.

I will surely try that cause this way was really tiring. I have a bunch of more ideas in my mind I will message in the slack.
Also ignore the commits for now, I will squash them once I am done.

[shivam-Purohit]

@spbnick
There are two argparse mentions in the code. One is for the original argparse and other is kcidb/argparse.

import argparse
import kcidb.misc
from kcidb import argparse

The action function uses the original argparse

class DBHelpAction(argparse.Action):
    """Argparse action outputting database string help and exiting."""
    def __init__(self,
                 option_strings,
                 dest=argparse.SUPPRESS,
                 default=argparse.SUPPRESS,
                 help=None,
                 ):
        super().__init__(
            option_strings=option_strings,
            dest=dest,
            default=default,
            nargs=0,
            help=help)

while the other argumentparser uses the kcidb/argparse.
class ArgumentParser(kcidb.argparse.ArgumentParser):

We were using kcidb.misc.argumentparser earlier but now if we use something like kcidb.argparse.ArgumentParser it confuses it with the original argparse. And returns error no attribute found ArgumentParser in argparse.

[shivam-Purohit]

ERROR STATEMENT

ImportError while loading conftest '/home/shivam/Desktop/outreachy/kcidb/conftest.py'.
conftest.py:7: in <module>
    import kcidb.db
kcidb/__init__.py:6: in <module>
    from kcidb.db.argparse import QueryArgumentParser
kcidb/db/__init__.py:9: in <module>
    from kcidb.db.argparse import QueryArgumentParser
kcidb/db/argparse.py:103: in <module>
    class OutputArgumentParser(kcidb.argparse.OutputArgumentParser):
E   AttributeError: partially initialized module 'kcidb' has no attribute 'argparse' (most likely due to a circular import)

[spbnick]

Sorry, I wasn't able to reproduce your problem. Please push the exact code you have problem with to this PR and specify how to reproduce the issue.

[shivam-Purohit]

class ArgumentParser(kcidb.argparse.ArgumentParser):
    """
    Command-line argument parser with common database arguments added.
    """

    def __init__(self, driver_types, *args, database=None, **kwargs):
        """
        Initialize the parser, adding common database arguments.

        Args:
            args:       Positional arguments to initialize ArgumentParser
                        with.
            database:   The default database specification to use, or None to
                        make database specification required.
            kwargs:     Keyword arguments to initialize ArgumentParser with.
        """
        self.DRIVER_TYPES = driver_types
        super().__init__(*args, **kwargs)
        add_args(self, database=database)

This is the ArgumentParser we defined in kcidb/db/argparse where constructor takes driver_types.
This db.ArgumentParser is also called in kcidb/argparse.
here we haven't passed driver_types are parameter.

parser = db.ArgumentParser(database="sqlite::memory:",
                              description=description)

This might be the reason for the error?

[spbnick]

This is the ArgumentParser we defined in kcidb/db/argparse where constructor takes driver_types.
This db.ArgumentParser is also called in kcidb/argparse.
here we haven't passed driver_types are parameter.

parser = db.ArgumentParser(database="sqlite::memory:", description=description)

This might be the reason for the error?

I doubt it. I have a hunch about what's going on, and I appreciate you trying to figure it out. However, if you want my help, please explain exactly how I can reproduce your problem, with the code in your PR. Thank you!

[shivam-Purohit]

@spbnick The way to reproduce will be then you will have to separate argparse in kcidb and kcidb/db. In db we are calling kcidb.argparse and in kcidb,argparse we are import that db.argparse.ArgumentParser. That may be the reason of circular import.
Before we weren't facing this issue because we had these functions in the third module mainly misc or init.py

If the error occurs due to a circular dependency, it can be resolved by moving the imported classes to a third file and importing them from this file.

PS: unless I finish this I cannot work on other as it is not allowing me to change branches ( i have build folder that is not committed)

[spbnick]

@spbnick The way to reproduce will be then you will have to separate argparse in kcidb and kcidb/db. In db we are calling kcidb.argparse and in kcidb,argparse we are import that db.argparse.ArgumentParser. That may be the reason of circular import.
Before we weren't facing this issue because we had these functions in the third module mainly misc or init.py

Could you push the exact code you have the problem with to this (or another) PR, and give me the exact command to execute to reproduce this? If this is still a problem, let's have a video call tomorrow, and go over this. Reach out to me on Slack and tell me the possible times for a call, if you'd like to do that.

PS: unless I finish this I cannot work on other as it is not allowing me to change branches ( i have build folder that is not committed)

Even though VS code could be making switching branches difficult, you could always clone the repository in another directory and work on another branch there.

[shivam-Purohit]

@spbnick Can you review?

[spbnick]

This is starting to take shape!

I left a bunch of comments/requests inline. Apart from them, please make sure your commits are in order, i.e. change-atomic and described appropriately, before review. This change seems to fit a single commit.

Thank you ❤️

[shivam-Purohit]

I don't know why the rebase changes my code (move back in time) it should be keeping the code the same and only merge commit into one. I will redo the changes and add separate commitsfor driver_types

[shivam-Purohit]

EDIT: i solved it the db.QueryArgumentParser was missing some command line options. You can look at the pr I also did the Pattern.Stringdoc change.

this is the error I was getting in the pytest. I could not trace the error. Could you help me with that @spbnick

Traceback (most recent call last):
  File "/home/shivam/Desktop/ahhhh/kcidb/test_kcidb.py", line 105, in test_query_main
    assert_executes("", *argv, driver_source=driver_source)
  File "/home/shivam/Desktop/ahhhh/kcidb/kcidb/unittest.py", line 90, in assert_executes
    raise AssertionError("\n".join(errors))
AssertionError: Expected exit status 0, got 1
Stderr doesn't match regex '':
    AttributeError: 'Namespace' object has no attribute 'issue_ids

[shivam-Purohit]

@spbnick I did it through the second method you told me on call. Can you review it?

[spbnick]

This is looking much better now! Thank you, Shivam!

Most of the inline comments are cosmetic, but there's one concerning a problem with logging-related symbols which would need another separate commit, and another regarding the completeness of driver_types change.

[spbnick]

These LOGGING_* symbols shouldn't be here, and they definitely shouldn't me simply copied here. They should be defined in one place only, to avoid desynchronization. I see that both the kcidb.misc and kcidb.argparse modules use those. To avoid both this copying and a circular import, we need to move all symbols starting with LOGGING_ or logging_ to their own module called kcidb.logging (in kcidb/logging.py file).

After moving the symbols should naturally lose their LOGGING_/logging_ prefixes, so e.g. kcidb.misc.LOGGING_LEVEL_MAP becomes kcidb.logging.LEVEL_MAP, and kcidb.misc.logging_setup becomes kcidb.logging.setup, and so on.

This change would better be done before extracting the argparse modules, in a separate commit.

[spbnick]

Please use the LOGGING_* symbols from kcidb.misc, instead of copying them here.

[shivam-Purohit]

@spbnick I forgot to add common in the argparse description I will add a commit

[spbnick]

Thank you, Shivam. I only have a bunch of minor requests. Otherwise this looks great.

Also, please "resolve conversation" for the requests that you implemented. Otherwise we'll be getting lost in them 🙈

Thanks!

[spbnick]

Please use the LOGGING_* symbols from kcidb.misc, instead of copying them here.

[shivam-Purohit]

@spbnick I have made the changes.
If it's okay we can work on # noqa. The commit should come after the separation of argparse right as it is unrelated to argparse change.

[spbnick]

If it's okay we can work on # noqa.

Great!

The commit should come after the separation of argparse right as it is unrelated to argparse change.

Doesn't really matter, as they're unrelated. Pick the order that's easier and creates less churn.

[shivam-Purohit]

@spbnick how bout this?

[spbnick]

Thank you, Shivam! We're almost there!

I found one more piece of unnecessarily-copied code, and three last stray changes, that need reverting.

To catch stray changes like that you could use a diff tool (e.g. diff itself, vimdiff, or something with a GUI). Get the file before your code move, and compare it to the file where you moved the code. E.g. like this:

vimdiff <(git show HEAD^^:kcidb/orm/__init__.py) kcidb/orm/argparse.py

[spbnick]

This should also stay in kcidb.misc, not get copied here. Import it from there and use like other modules do. E.g. kcidb.oo.

Generally, you should avoid duplicating code and definitions.

[spbnick]

Please revert this change of double quotes to single quotes:

Suggested change

	nargs='*',
	nargs="*",

It's unrelated to this commit.

[spbnick]

Same here, please revert this formatting change, and restore the comma on the end, in this commit:

Suggested change

	"See pattern documentation with --pattern-help."
	"See pattern documentation with --pattern-help.",

Please avoid making unrelated changes, especially when moving code around. It complicates reviews.

[spbnick]

Similarly here:

Suggested change

	help="Print pattern string documentation and exit."
	help="Print pattern string documentation and exit.",