Skip to content

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

Open
lpozo wants to merge 1 commit into
ilstam:masterfrom
lpozo:master
Open

Simplify utils.add_to_layout() avoiding the use of type checking#60
lpozo wants to merge 1 commit into
ilstam:masterfrom
lpozo:master

Conversation

@lpozo
Copy link
Copy Markdown

@lpozo lpozo commented Aug 13, 2018

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

@ilstam
Copy link
Copy Markdown
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
Copy Markdown
Author

lpozo commented Aug 13, 2018

Do you really want someone to collaborate with this project?

@ilstam
Copy link
Copy Markdown
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