New submission from Maciej Szulik:
Anish here are my review comments:
1. detectors/github_pullrequest_url.py
Add an empty line before imports.
> if parsed_url.scheme not in ('http', 'https'):
You're expecting http or https, but what if I paste
github.com/python/cpython/pull/41 which is perfectly valid but will fail your
validator?
You can actually add http on your own if it's missing, for purity ;)
> newvalues['url'] = (parsed_url.scheme + "://" + parsed_url.netloc +
> parsed_url.path)
Why do you need to reconstruct the url in this case? Why not just assigning the
one you verified is ok?
iow. newvalues['url'] = url
> github_url_id = db.github_pullrequest_url.lookup(pullrequest_number)
No need to assign to a value if you're not using it.
2. html/issue.item.html
> <th><tal:block i18n:translate="">GitHub Pull Request URL</tal:block>:</th>
The name is lengthy, make it shorter 'GitHub PR' should be enough, imho.
Don't forget to change error messages so they reflect that change as well.
> <th i18n:translate="">Pull Request</th>
Change to Link.
> <a tal:attributes="href github_pullrequest_url/url" tal:define="PR
> github_pullrequest_url/pullrequest_number" tal:content="python:'PR %s' % PR"
> target="_blank">
Put entire link, it won't be long, and the short version looks awful.
> <a tal:condition="github_pullrequest_url/is_edit_ok" tal:attributes="href
> string:github_pullrequest_url${github_pullrequest_url/id}">edit</a>
It would be nice to also have Remove button similarly to how we have with
files, hgrepos, etc. See the files section on this page, as an example.
Additionally there's an empty line right after this cell, please remove ;)
3. schema.py
> github_pullrequest_url = Class(db, "github_pullrequest_url",
Make shorter, 'github_prs' is enough.
> pullrequest_number=String(),
'id', 'no', 'pr_no' one of them will be sufficient, as well.
Have we discussed the possibility of having the links being crossed when the PR
is merged or at least having a separate column with the state, I think we did.
Can you link the issue if one exists?
_______________________________________________________
PSF Meta Tracker <metatrac...@psf.upfronthosting.co.za>
<http://psf.upfronthosting.co.za/roundup/meta/issue586>
_______________________________________________________
_______________________________________________
Tracker-discuss mailing list
Tracker-discuss@python.org
https://mail.python.org/mailman/listinfo/tracker-discuss
Code of Conduct: https://www.python.org/psf/codeofconduct/
