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

type annotate backtrack_depgraph in depgraph.py #1031

Closed
wants to merge 7 commits into from
Closed

type annotate backtrack_depgraph in depgraph.py #1031

wants to merge 7 commits into from

Conversation

berinaniesh
Copy link
Contributor

No description provided.

@@ -11385,7 +11387,7 @@ def _spinner_stop(spinner):
portage.writemsg_stdout(f"Dependency resolution took {darkgreen(time_fmt)} s.\n\n")


def backtrack_depgraph(settings, trees, myopts, myparams, myaction, myfiles, spinner):
def backtrack_depgraph(settings: portage.package.ebuild.config.config, trees: dict, myopts: dict, myparams: dict, myaction, myfiles: [str], spinner: stdout_spinner) -> (bool, depgraph, [str]):
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to quote the type to avoid an import here:

Suggested change
def backtrack_depgraph(settings: portage.package.ebuild.config.config, trees: dict, myopts: dict, myparams: dict, myaction, myfiles: [str], spinner: stdout_spinner) -> (bool, depgraph, [str]):
def backtrack_depgraph(settings: "portage.package.ebuild.config.config", trees: dict, myopts: dict, myparams: dict, myaction, myfiles: [str], spinner: stdout_spinner) -> (bool, depgraph, [str]):

Also, when you run mypy before & afterwards, do you get many more errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quotes to avoid import is very useful I guess. So, we can remove the import of stdout_spinner and use quotes again.
mypy says "Found 115 errors in 43 files (checked 1 source file)", when checking depgraph.py.

Copy link
Member

@thesamesam thesamesam May 1, 2023

Choose a reason for hiding this comment

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

How many errors did it give before your change, vs after? This can both be a guide for more work to do or a sign you've done something wrong, depending on what they are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not run mypy before. After the commit, it says 115 errors. I ran mypy now without the commit, it says "Found 111 errors in 42 files (checked 1 source file)".

Copy link
Member

@thesamesam thesamesam May 1, 2023

Choose a reason for hiding this comment

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

I'm asking you to run it before this commit now, if you haven't already, and compare the output (ideally showing both here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the output of mypy depgraph.py before the commit. https://dpaste.com/2X6BTB24X
This is the output of mypy depgraph.py after the commit. https://dpaste.com/BK8MNCWFN

Copy link
Member

@thesamesam thesamesam May 1, 2023

Choose a reason for hiding this comment

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

Okay, so this introduces 4 new mypy errors (111 vs 115). Specifically:

lib/_emerge/depgraph.py:11390: error: Bracketed expression "[...]" is not valid as a type  [valid-type]
lib/_emerge/depgraph.py:11390: note: Did you mean "List[...]"?
lib/_emerge/depgraph.py:11390: error: Syntax error in type annotation  [syntax]
lib/_emerge/depgraph.py:11390: note: Suggestion: Use Tuple[T1, ..., Tn] instead of (T1, ..., Tn)

You'll need to fix those.

@berinaniesh
Copy link
Contributor Author

berinaniesh commented May 1, 2023

@thesamesam I think I rebased my fork with gentoo/portage.
The new commits bring in two new errors from mypy. https://dpaste.com/2CU8ZU7UC
The diff can be viewed here. https://dpaste.com/2CU8ZU7UC/diff/2X6BTB24X

@thesamesam
Copy link
Member

Looks like it fixes 2 errors which is good!

@berinaniesh
Copy link
Contributor Author

Looks like it fixes 2 errors which is good!

Is there anything else I should change?

@thesamesam
Copy link
Member

mypy lib/_emerge/depgraph.py says:

[...]
lib/_emerge/depgraph.py:11396: error: Bracketed expression "[...]" is not valid as a type  [valid-type]
lib/_emerge/depgraph.py:11396: note: Did you mean "List[...]"?
lib/_emerge/depgraph.py:11398: error: Syntax error in type annotation  [syntax]
lib/_emerge/depgraph.py:11398: note: Suggestion: Use Tuple[T1, ..., Tn] instead of (T1, ..., Tn)

Please make sure to test your changes thoroughly. Also, black is unhappy with the formatting.

@berinaniesh
Copy link
Contributor Author

You said these last time, I changed them. Won't the new commits automatically come into the pull request?

@thesamesam
Copy link
Member

I think however you're pushing isn't right.

git rebase origin/master then git push -f should work.

@berinaniesh
Copy link
Contributor Author

I did and it shows "Everything up-to-date". I didn't make a branch before making changes. That hinders my other works as well. I'll close this pull request, make a branch, make few more type annotations and submit a new one @thesamesam.

@thesamesam
Copy link
Member

OK, sounds good.

I recommend that:

  • you have origin as github.com/gentoo/portage (or anongit.gentoo.org etc)
  • you have berin or me or whatever as github.com/berinaniesh/portage
  • you run git config --global pull.rebase true to avoid merge commits being generated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants