This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11912

pytype: stop excluding changegroup.py
ClosedPublic

Authored by mharbison72 on Dec 14 2021, 4:10 PM.

Download Raw Diff

Details

Reviewers

Alphare

Group Reviewers

hg-reviewers

Commits

rHG8843c9a8771b: pytype: stop excluding changegroup.py

Summary

The false positives that were detected seem to be related to what happens to the
variables after the local methods are used:

File "/mnt/c/Users/Matt/hg/mercurial/changegroup.py", line 353, in ondupchangelog:
    No attribute 'append' on range [attribute-error]
  In Union[List[nothing], range]
File "/mnt/c/Users/Matt/hg/mercurial/changegroup.py", line 357, in onchangelog:
    No attribute 'update' on None [attribute-error]

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mharbison72 created this revision.Dec 14 2021, 4:10 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptDec 14 2021, 4:10 PM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

mharbison72 added a child revision: D11913: pytype: stop excluding lsprof.py.Dec 14 2021, 4:10 PM

Alphare requested changes to this revision.Dec 15 2021, 5:01 AM

Alphare added a subscriber: Alphare.

Alphare added inline comments.

mercurial/changegroup.py
536	Mhhh now it just looks like nonsensical code. I don't think we should appease Pytype this way

This revision now requires changes to proceed.Dec 15 2021, 5:01 AM

mharbison72 added inline comments.Dec 15 2021, 2:45 PM

mercurial/changegroup.py
536	I didn't like it either, so I'll just make it suppression comments. FTR, I'm a little concerned about all of the suppression comments (in general, not necessarily here) in case the code changes in a way that it is an issue, but the comments hide it. Also, reusing variables (like `foo = str2bytes(foo)`) tends to confuse PyCharm, which is a shame because it does a really good job of finding issues. I'm not sure where the sweet spot is.

mharbison72 updated this revision to Diff 31497.Dec 15 2021, 4:19 PM

Alphare accepted this revision.Dec 17 2021, 5:57 AM

Alphare added inline comments.

mercurial/changegroup.py
536	I think these will become easier with the Python 2 removal. We can talk about it more then. :)

This revision is now accepted and ready to land.Dec 17 2021, 5:57 AM

mharbison72 added a commit: rHG8843c9a8771b: pytype: stop excluding changegroup.py.Dec 17 2021, 6:09 AM

Closed by commit rHG8843c9a8771b: pytype: stop excluding changegroup.py (authored by mharbison72). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/changegroup.py (3 lines)
M			tests/test-check-pytype.t (2 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	31469		Dec 14 2021, 4:10 PM	★	★
Diff 2	31497		Dec 15 2021, 4:19 PM	★	★
Diff 3	31506	rHG8843c9a8771be9101bf96c482ac10aa53bfffbed	Dec 12 2021, 9:10 PM	★	★

Status	Author	Revision
Closed	mharbison72	D11924 pytype: stop excluding webcommands.py
Closed	mharbison72	D11923 tests: drop an obsolete comment about a pytype failure
Closed	mharbison72	D11922 pytype: stop excluding patch.py
Closed	mharbison72	D11921 pytype: stop excluding statprof.py
Closed	mharbison72	D11920 statprof: convert a few exception byte strings to str
Closed	mharbison72	D11919 pytype: stop excluding wireprotov2server.py
Closed	mharbison72	D11918 pytype: stop excluding stringutil.py
Closed	mharbison72	D11917 pytype: stop excluding cmdutil.py
Closed	mharbison72	D11916 pytype: stop excluding copies.py
Closed	mharbison72	D11915 copies: fix some documentation typos
Closed	mharbison72	D11914 copies: drop an unused variable
Closed	mharbison72	D11913 pytype: stop excluding lsprof.py
Closed	mharbison72	D11912 pytype: stop excluding changegroup.py
Closed	mharbison72	D11911 pytype: stop excluding upgrade.py

Diff 31506

mercurial/changegroup.py

	)			)
	self.callback = progress.increment			self.callback = progress.increment

	efilesset = set()			efilesset = set()
	duprevs = []			duprevs = []

	def ondupchangelog(cl, rev):			def ondupchangelog(cl, rev):
	if rev < clstart:			if rev < clstart:
	duprevs.append(rev)			duprevs.append(rev) # pytype: disable=attribute-error

	def onchangelog(cl, rev):			def onchangelog(cl, rev):
	ctx = cl.changelogrevision(rev)			ctx = cl.changelogrevision(rev)
				assert efilesset is not None # help pytype
	efilesset.update(ctx.files)			efilesset.update(ctx.files)
	repo.register_changeset(rev, ctx)			repo.register_changeset(rev, ctx)

	self.changelogheader()			self.changelogheader()
	deltas = self.deltaiter()			deltas = self.deltaiter()
	if not cl.addgroup(			if not cl.addgroup(
	deltas,			deltas,
	csmap,			csmap,
	# outside, their phases are going to be pushed			# outside, their phases are going to be pushed
	# alongside. Therefor `targetphase` is			# alongside. Therefor `targetphase` is
	# ignored.			# ignored.
	targetphase = phaseall = phases.draft			targetphase = phaseall = phases.draft
	if added:			if added:
	phases.registernew(repo, tr, targetphase, added)			phases.registernew(repo, tr, targetphase, added)
	if phaseall is not None:			if phaseall is not None:
	if duprevs:			if duprevs:
	duprevs.extend(added)			duprevs.extend(added)
				AlphareUnsubmitted Not Done Mhhh now it just looks like nonsensical code. I don't think we should appease Pytype this way Alphare: Mhhh now it just looks like nonsensical code. I don't think we should appease Pytype this way
				mharbison72AuthorUnsubmitted Done I didn't like it either, so I'll just make it suppression comments. FTR, I'm a little concerned about all of the suppression comments (in general, not necessarily here) in case the code changes in a way that it is an issue, but the comments hide it. Also, reusing variables (like `foo = str2bytes(foo)`) tends to confuse PyCharm, which is a shame because it does a really good job of finding issues. I'm not sure where the sweet spot is. mharbison72: I didn't like it either, so I'll just make it suppression comments. FTR, I'm a little…
				AlphareUnsubmitted Not Done I think these will become easier with the Python 2 removal. We can talk about it more then. :) Alphare: I think these will become easier with the Python 2 removal. We can talk about it more then. :)
	else:			else:
	duprevs = added			duprevs = added
	phases.advanceboundary(repo, tr, phaseall, [], revs=duprevs)			phases.advanceboundary(repo, tr, phaseall, [], revs=duprevs)
	duprevs = []			duprevs = []

	if changesets > 0:			if changesets > 0:

	def runhooks(unused_success):			def runhooks(unused_success):

tests/test-check-pytype.t

	#require pytype py3 slow			#require pytype py3 slow

	$ cd $RUNTESTDIR/..			$ cd $RUNTESTDIR/..

	Many of the individual files that are excluded here confuse pytype			Many of the individual files that are excluded here confuse pytype
	because they do a mix of Python 2 and Python 3 things			because they do a mix of Python 2 and Python 3 things
	conditionally. There's no good way to help it out with that as far as			conditionally. There's no good way to help it out with that as far as
	I can tell, so let's just hide those files from it for now. We should			I can tell, so let's just hide those files from it for now. We should
	endeavor to empty this list out over time, as some of these are			endeavor to empty this list out over time, as some of these are
	probably hiding real problems.			probably hiding real problems.

	mercurial/bundlerepo.py # no vfs and ui attrs on bundlerepo			mercurial/bundlerepo.py # no vfs and ui attrs on bundlerepo
	mercurial/changegroup.py # mysterious incorrect type detection
	mercurial/chgserver.py # [attribute-error]			mercurial/chgserver.py # [attribute-error]
	mercurial/cmdutil.py # No attribute 'markcopied' on mercurial.context.filectx [attribute-error]			mercurial/cmdutil.py # No attribute 'markcopied' on mercurial.context.filectx [attribute-error]
	mercurial/context.py # many [attribute-error]			mercurial/context.py # many [attribute-error]
	mercurial/copies.py # No attribute 'items' on None [attribute-error]			mercurial/copies.py # No attribute 'items' on None [attribute-error]
	mercurial/crecord.py # tons of [attribute-error], [module-attr]			mercurial/crecord.py # tons of [attribute-error], [module-attr]
	mercurial/debugcommands.py # [wrong-arg-types]			mercurial/debugcommands.py # [wrong-arg-types]
	mercurial/dispatch.py # initstdio: No attribute ... on TextIO [attribute-error]			mercurial/dispatch.py # initstdio: No attribute ... on TextIO [attribute-error]
	mercurial/exchange.py # [attribute-error]			mercurial/exchange.py # [attribute-error]
	mercurial/wireprotov1server.py # BUG?: BundleValueError handler accesses subclass's attrs			mercurial/wireprotov1server.py # BUG?: BundleValueError handler accesses subclass's attrs
	mercurial/wireprotov2server.py # [unsupported-operands], [attribute-error]			mercurial/wireprotov2server.py # [unsupported-operands], [attribute-error]

	TODO: use --no-cache on test server? Caching the files locally helps during			TODO: use --no-cache on test server? Caching the files locally helps during
	development, but may be a hinderance for CI testing.			development, but may be a hinderance for CI testing.

	$ pytype -V 3.6 --keep-going --jobs auto mercurial \			$ pytype -V 3.6 --keep-going --jobs auto mercurial \
	> -x mercurial/bundlerepo.py \			> -x mercurial/bundlerepo.py \
	> -x mercurial/changegroup.py \
	> -x mercurial/chgserver.py \			> -x mercurial/chgserver.py \
	> -x mercurial/cmdutil.py \			> -x mercurial/cmdutil.py \
	> -x mercurial/context.py \			> -x mercurial/context.py \
	> -x mercurial/copies.py \			> -x mercurial/copies.py \
	> -x mercurial/crecord.py \			> -x mercurial/crecord.py \
	> -x mercurial/debugcommands.py \			> -x mercurial/debugcommands.py \
	> -x mercurial/dispatch.py \			> -x mercurial/dispatch.py \
	> -x mercurial/exchange.py \			> -x mercurial/exchange.py \