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

Fixes #1627 - Adds coco.fr to the abuse list #1628

Merged
merged 1 commit into from
Jul 11, 2017

Conversation

karlcow
Copy link
Member

@karlcow karlcow commented Jun 30, 2017

r? @cch5ng

We had abuse on the project for the domain coco.fr.

@karlcow karlcow requested a review from cch5ng June 30, 2017 16:16
@cch5ng
Copy link
Contributor

cch5ng commented Jul 3, 2017

@karlcow sorry for the delay, I will look at this today

Copy link
Contributor

@cch5ng cch5ng left a comment

Choose a reason for hiding this comment

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

(updated) LGTM in python. seems like a bunch of travis tests are failing though. is that important? after addressing the travis issues, this could be merged.

it will take me additional time to go through the views.py code in detail to ask questions. but it seems I could do so even after the PR gets closed.

@karlcow
Copy link
Member Author

karlcow commented Jul 3, 2017

@cch5ng it's good you took time. The storm seems to have passed. I will close the pull request and we can reopen it, in case this is coming back.

Thanks a lot.

@cch5ng
Copy link
Contributor

cch5ng commented Jul 5, 2017

@karlcow this is the start to my questions about views.py (for /issues/new route) but eventually I'll cover the entire views.py. I might include some questions I had when trying to address a different issue later.

for https://github.com/webcompat/webcompat.com/blob/master/webcompat/views.py#L179
should set() be updated to frozenset()? I had to look up issubset() and saw a deprecation notice in the docs. https://docs.python.org/2/library/sets.html ... but am not totally clear about it

https://github.com/webcompat/webcompat.com/blob/master/config/__init__.py#L49
(this is something I looked at for a different issue) why do you use a namedtuple here? is it just to be more descriptive?

for the homepage view (/), I'm trying to understand the flow for how the triage issues list gets generated. in the source it seems to be coming from a backbone model but I'm not clear where in the code the request to the github api gets made. could I get a pointer to which source files I should be looking at?

in the future is it OK for me to include both you and MikeT as JS reviewers? I know both of you are very busy but your review concerns are a bit different and I think I learn from both forms.

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

Successfully merging this pull request may close these issues.

3 participants