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

For rg3 deposit #31

Closed
wants to merge 1 commit into from
Closed

For rg3 deposit #31

wants to merge 1 commit into from

Conversation

vvavrychuk
Copy link
Contributor

No description provided.

@rg3
Copy link
Collaborator

rg3 commented Dec 5, 2010

I don't have time to review this today. I'll try to take a look at your code tomorrow.

@rg3
Copy link
Collaborator

rg3 commented Dec 8, 2010

This needs a few changes to be merged. I'm adding comments to the code so you can redo part of that work. Also, you need to agree to maintain that InfoExtractor for me to merge it. :)

@rg3
Copy link
Collaborator

rg3 commented Dec 8, 2010

I just finished commenting your commit. In addition to the comments I made, a general complaint: you'd really need to use tabs for indentation of the new code as the rest of the program. Indentation needs to be consistent, and I made the choice of tabs this time.

@vvavrychuk
Copy link
Contributor Author

Thanks for valuable review. I've made suggested changes and I agree to be maint. For tabs I already use them.

@rg3
Copy link
Collaborator

rg3 commented Dec 8, 2010

Oh, sorry about the tabs. The web interface represents them as two spaces and I was not aware all the code looked like that. My bad. :)

@vvavrychuk
Copy link
Contributor Author

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants