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

Simplify utils.add_to_layout() avoiding the use of type checking #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lpozo
Copy link

@lpozo lpozo commented Aug 13, 2018

Simplify utils.add_to_layout() avoiding the use of type checking and improving performance

@ilstam
Copy link
Owner

ilstam commented Aug 13, 2018

Hello there.

I don't understand why you want to replace the isinstance calls by manually checking the class's names. You're trying to do the same thing only in an unreliable and non-standard way.

Also by removing the final else clause that raises the TypeError you assume that the caller passed a proper type. And that is not a good assumption.

Finally, I don't have a strong opinion about passing strings as the first arg to add_to_layout. I reckon that I did it that way in order for the calls to this function to be shorter in length as it is called frequently. Why do you propose that it should be removed and the caller should explicitly pass a layout class?

Thanks.

@lpozo
Copy link
Author

lpozo commented Aug 13, 2018

Do you really want someone to collaborate with this project?

@ilstam
Copy link
Owner

ilstam commented Aug 13, 2018

I would be happy to accept patches that improve the source code's quality or implement new functionality.

However, I raised my concerns and questions about your changes and you're not really giving any answers.

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