Code review comment for lp:~james-w/launchpad/register-code-import

On Tue, 06 Apr 2010 15:33:31 -0000, Aaron Bentley <email address hidden> wrote:
> On 04/05/2010 05:39 PM, James Westby wrote:
>
> >> Was there any lint?
> >
> > I don't know, running "make lint" runs it on lots of unexpected files.
>
> When there are no uncommitted changes, "make lint" compares your tree to
> the parent branch to determine which files it runs against. This means
> it will also lint files changed by your prerequisite branch. You parent
> branch may also be out of date.

I am using pipelines, so I would have thought it would run against the
previous pipe?

> You can also run bin/lint.sh with the list of files you modified. If
> you use pipelines and have the lp-review-body plugin installed, you can
> use lp-propose (formerly lp-submit) and it will use the previous pipe to
> determine which files to lint.

That's what I used (at least for one of these proposals,) but I didn't
have the extra plugin installed.

> lib/canonical/launchpad/interfaces/_schema_circular_imports.py
> 43: 'IBranchTarget' imported but unused

Fixed.

> lib/lp/code/interfaces/branch.py
> 11: undefined name 'UnknownBranchTypeError' in __all__

Fixed.

> lib/lp/code/interfaces/webservice.py
> 8: 'BranchMergeProposalExists' imported but unused
> 9: 'BranchCreatorNotOwner' imported but unused
> 9: 'IBranchSet' imported but unused
> 9: 'IBranch' imported but unused
> 9: 'BranchExists' imported but unused
> 9: 'BranchCreatorNotMemberOfOwnerTeam' imported but unused
> 12: 'IBranchMergeProposal' imported but unused
> 13: 'IBranchSubscription' imported but unused
> 14: 'ICodeImport' imported but unused
> 15: 'ICodeReviewComment' imported but unused
> 16: 'ICodeReviewVoteReference' imported but unused
> 17: 'IStaticDiff' imported but unused
> 17: 'IDiff' imported but unused
> 17: 'IPreviewDiff' imported but unused

Noise. This file is purely for importing.

> lib/lp/registry/model/sourcepackage.py
> 684: local variable 'displayname' is assigned to but never used

Fixed.

> lib/lp/code/interfaces/branch.py
> 442: [C0322, IBranch.setOwner] Operator not preceded by a space
> title=_("The new owner of the branch."),
> ^
> schema=IPerson))
> @export_write_operation()
> def setOwner(new_owner, user):
> 451: [C0322, IBranch.setTarget] Operator not preceded by a space
> title=_("The project the branch belongs to."),
> ^
> schema=Interface, required=False), # Really IProduct
> source_package=Reference(
> title=_("The source package the branch belongs to."),
> schema=Interface, required=False)) # Really ISourcePackage
> @export_write_operation()
> def setTarget(user, project=None, source_package=None):
> 511: [C0322, IBranch.isPersonTrustedReviewer] Operator not preceded
> by a space
> title=_("A person for which the reviewer status is in question."),
> ^
> schema=IPerson))
> @export_read_operation()
> def isPersonTrustedReviewer(reviewer):
> 760: [C0322, IBranch._createMergeProposal] Operator not preceded by
> a space
> needs_review=Bool(title=_('Needs review'),
> ^
> description=_('If True the proposal needs review.'
> 'Otherwise, it will be work in progress.')),
> initial_comment=Text(
> title=_('Initial comment'),
> description=_("Registrant's initial description of proposal.")),
> commit_message=Text(
> title=_('Commit message'),
> description=_('Message to use when committing this merge.')),
> reviewers=List(value_type=Reference(schema=IPerson)),
> review_types=List(value_type=TextLine())
> )
>
>
> @call_with(registrant=REQUEST_USER)
>
> @export_factory_operation(Interface, [])
> def _createMergeProposal(
> registrant, target_branch, prerequisite_branch=None,
> needs_review=True, initial_comment=None, commit_message=None,
> reviewers=None, review_types=None):
> 937: [C0322, IBranch.subscribe] Operator not preceded by a space
> schema=IPerson),
> ^
> notification_level=Choice(
> title=_("The level of notification to subscribe to."),
> vocabulary=BranchSubscriptionNotificationLevel),
> max_diff_lines=Choice(
> title=_("The max number of lines for diff email."),
> vocabulary=BranchSubscriptionDiffSize),
> code_review_level=Choice(
> title=_("The level of code review notification emails."),
> vocabulary=CodeReviewNotificationLevel))
> @operation_returns_entry(Interface) # Really IBranchSubscription
> @export_write_operation()
> def subscribe(person, notification_level, max_diff_lines,
> code_review_level):
> 965: [C0322, IBranch.getSubscription] Operator not preceded by a space
> schema=IPerson))
> ^
> @operation_returns_entry(Interface) # Really IBranchSubscription
> @export_read_operation()
> def getSubscription(person):
> 976: [C0322, IBranch.unsubscribe] Operator not preceded by a space
> title=_("The person to unsubscribe"),
> ^
> schema=IPerson))
> @export_write_operation()
> def unsubscribe(person):
> 1226: [C0322, IBranchSet.getByUrls] Operator not preceded by a space
> title=u'A list of URLs of branches',
> ^
> description=(
> u'These can be URLs external to '
> u'Launchpad, lp: URLs, or http://bazaar.launchpad.net/ URLs, '
> u'or any mix of all these different kinds.'),
> value_type=TextLine(),
> required=True))
> @export_read_operation()
> def getByUrls(urls):

Noise.

> lib/lp/code/interfaces/hasbranches.py
> 104: [C0301] Line too long (79/78)

Fixed.

> 42: [E0213, IHasBranches.getBranches] Method should have "self" as
> first argument
> 42: [C0322, IHasBranches.getBranches] Operator not preceded by a space
> modified_since=Datetime(
> ^
> title=_('Limit the branches to those modified since this date.'),
> required=False))
> @call_with(visible_by_user=REQUEST_USER)
> @operation_returns_collection_of(Interface) # Really IBranch.
> @export_read_operation()
> def getBranches(status=None, visible_by_user=None,
> modified_since=None):
> 74: [E0213, IHasMergeProposals.getMergeProposals] Method should
> have "self" as first argument
> 100: [E0213, IHasRequestedReviews.getRequestedReviews] Method
> should have "self" as first argument
> 128: [E0213, IHasCodeImports.newCodeImport] Method should have
> "self" as first argument
> 128: [C0322, IHasCodeImports.newCodeImport] Operator not preceded
> by a space
> rcs_type=Choice(vocabulary=RevisionControlSystems, required=True),
> ^
> url=TextLine(title=_('Foreign VCS URL')),
> cvs_root=TextLine(title=_('CVS root URL')),
> cvs_module=TextLine(title=_('CVS module to import')),
> owner=Reference(title=_('Owner of the resulting branch'),
> schema=Interface)
> )
> @call_with(registrant=REQUEST_USER)
> @export_factory_operation(Interface, []) # Really ICodeImport.
> def newCodeImport(registrant=None, branch_name=None, rcs_type=None,
> url=None, cvs_root=None, cvs_module=None, owner=None):

Noise.

> lib/lp/code/model/branchtarget.py
> 328: [W0231, ProductSeriesBranchTarget.__init__] __init__ method
> from base class 'ProductBranchTarget' is not called

Fixed.

> lib/lp/code/model/hasbranches.py
> 64: [C0301] Line too long (79/78)

Fixed

> lib/lp/registry/interfaces/product.py
> 774: [C0322, IProductSet.createProduct] Operator not preceded by a
> space
> freshmeatproject='freshmeat_project', wikiurl='wiki_url',
> ^
> downloadurl='download_url',
> sourceforgeproject='sourceforge_project',
> programminglang='programming_lang')
> @export_factory_operation(
> IProduct, ['name', 'displayname', 'title', 'summary', 'description',
> 'project', 'homepageurl', 'screenshotsurl',
> 'downloadurl', 'freshmeatproject', 'wikiurl',
> 'sourceforgeproject', 'programminglang',
> 'license_reviewed', 'licenses', 'license_info',
> 'registrant'])
> @export_operation_as('new_project')
> def createProduct(owner, name, displayname, title, summary,
> description=None, project=None, homepageurl=None,
> screenshotsurl=None, wikiurl=None,
> downloadurl=None, freshmeatproject=None,
> sourceforgeproject=None, programminglang=None,
> license_reviewed=False, mugshot=None, logo=None,
> icon=None, licenses=None, license_info=None,
> registrant=None):

Noise.

> lib/lp/registry/interfaces/sourcepackage.py
> 189: [C0322, ISourcePackage.getBranch] Operator not preceded by a space
> vocabulary=DBEnumeratedType))
> ^
>
>
> @operation_returns_entry(Interface)
> @export_read_operation()
> def getBranch(pocket):
> 207: [C0322, ISourcePackage.setBranch] Operator not preceded by a space
> vocabulary=DBEnumeratedType),
> ^
> branch=Reference(Interface, title=_("Branch"), required=False))
> @call_with(registrant=REQUEST_USER)
> @export_write_operation()
> def setBranch(pocket, branch, registrant):

Noise.

> Some of these may be bogus, but many of them should be fixed. You
> should suppress the pylint warnings in lib/lp/code/interfaces/hasbranches.py

Done.

> >> What would you think about expressing xx-code-import.txt's "Exceptions"
> >> heading as a unit test? Failing that, it needs two blank lines separating it
> >> from the previous section.
> >
> > I don't know how to do webservice tests as unit tests, if you can point me to
> > examples I will convert them.
>
> What works in doctests will usually work in unit tests:

Thanks, changed.

James