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

Break down kcidb.orm module #396

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Break down kcidb.orm module #396

merged 1 commit into from
Mar 29, 2023

Conversation

octonawish-akcodes
Copy link
Contributor

@octonawish-akcodes octonawish-akcodes commented Mar 17, 2023

Extracted Schema and Pattern class and their dependencies and made them as a separate module from kcidb.orm module.
This PR closes #386
@spbnick Need reviews.

@octonawish-akcodes octonawish-akcodes marked this pull request as draft March 17, 2023 02:18
Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Nice start, Abhishek! Left you a few comments/requests inline. Thanks!

kcidb/orm/data.py Outdated Show resolved Hide resolved
kcidb/orm/query.py Outdated Show resolved Hide resolved
kcidb/orm/query.py Outdated Show resolved Hide resolved
@spbnick
Copy link
Collaborator

spbnick commented Mar 17, 2023

Don't hesitate to ask any questions you might have about this, here, or on Slack!

@octonawish-akcodes octonawish-akcodes marked this pull request as ready for review March 20, 2023 11:18
@octonawish-akcodes
Copy link
Contributor Author

octonawish-akcodes commented Mar 20, 2023

After making the suggested changes, Below is the output of the test cases. All are passing as they were passing before creating the separate modules for SCHEMA and Pattern.

image

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Please keep reformatting changes separate from actual code changes. Otherwise it is too hard to review, rebase, merge, and find problems in such changes. That is, do not reformat code you're not changing in the same commit. If you want to change formatting for whatever reason, please make that a separate commit and explain what you're doing and why.

Please remove, or separate pure reformatting changes from the actual code changes and then I will review this PR. Especially please make sure you do not reformat the code you're moving, as that makes it hard to see what stayed the same and what changed. Thank you ❤️

@octonawish-akcodes
Copy link
Contributor Author

octonawish-akcodes commented Mar 21, 2023

In this updated PR, I removed the reformatting, I'll make a separate PR for that if necessary. Now, this PR is purely the breakdown changes to be reviewed @spbnick

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Very nice! We're close to the finish line here!

Please deal with the CI errors, and from now on please make sure CI passes before review, as you're now more experienced with the project (please keep ignoring the deployment failures for now).

Please also don't forget to keep rebasing your changes on latest main as you work, and before reviews.

Finally, please explain a little what you're doing in the commit message body (take the issue description as a reference).

@octonawish-akcodes
Copy link
Contributor Author

Test cases passed, fixes the cyclic imports, and updated the references where necessary. I also added a description of what I am doing and what I have done have a look, sir. This PR is intended to work as we wanted it to, need reviews @spbnick

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Ah, very neat and tidy ☺️ Thank you, Abhishek!

I only have a few minor requests left.

The commit message looks very good too, thanks!

It would be totally perfect if you used the imperative mood in the commit body as well (i.e. wrote "Extract parts ...", instead of "We extracted parts ...", and so on), but the most important part is the mood of the commit subject, and that's fine.

kcidb/db/abstract.py Outdated Show resolved Hide resolved
kcidb/orm/data.py Outdated Show resolved Hide resolved
kcidb/orm/query.py Outdated Show resolved Hide resolved
@octonawish-akcodes
Copy link
Contributor Author

@spbnick Done all the requested changes, Need reviews.

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Thank you very much Abhishek! This looks very good and satisfies all my previous requests.

However, now that your PR is much neater I was able to notice one small, and one larger problem with this. I left you a couple requests inline.

Thank you.

kcidb/orm/__init__.py Outdated Show resolved Hide resolved
kcidb/orm/__init__.py Outdated Show resolved Hide resolved
@spbnick
Copy link
Collaborator

spbnick commented Mar 23, 2023

@octonawish-akcodes, I tried reproducing your import problems, and it would go smoother if instead of things like this:

import kcidb.orm.query as query
import kcidb.orm.query as data

you would write

from kcidb.orm import query, data

@octonawish-akcodes
Copy link
Contributor Author

octonawish-akcodes commented Mar 28, 2023

@octonawish-akcodes, I tried reproducing your import problems, and it would go smoother if instead of things like this:

import kcidb.orm.query as query
import kcidb.orm.query as data

you would write

from kcidb.orm import query, data

All requested changes have been implemented and the tests are passing in CI @spbnick Need reviews.

@spbnick
Copy link
Collaborator

spbnick commented Mar 28, 2023

We don't have much time left for you to clean this up, and we're blocking #379, so I went ahead and did it for you. Please review the patch I added on top of yours, then squash them together, push to PR, and then review the result again.

Main changes I did:

  • Remove all unnecessary changes, including whitespace and import changes. It's generally better to minimize churn and do only what's necessary for a particular task (provided it's done well). We can work on unifying and refining imports later.
  • Update documentation so it refers to the new location of the moved symbols. Always remember to update the docs.
  • Remove the LOGGER global from modules that don't need it. Do not add globals unnecessarily.

Extract parts of kcidb.orm module into separate modules for improved
maintainability and readability. Specifically, we refactored the module by
creating two new modules - kcidb.orm.data for SCHEMA and dependencies, and
kcidb.orm.query for Pattern and dependencies. Updated all relevant
references to the new modules. No changes in behavior.

Signed-off-by: Abhishek Kumar <[email protected]>
@octonawish-akcodes
Copy link
Contributor Author

@spbnick Have a look now?

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you, @octonawish-akcodes. Merging!

@spbnick spbnick merged commit a217ae7 into kernelci:main Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Break down kcidb.orm module
2 participants