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

DOC: remove not accepted arguments from pl.scatter #557

Merged
merged 2 commits into from Mar 28, 2019

Conversation

Projects
None yet
2 participants
@fabianrost84
Copy link
Contributor

fabianrost84 commented Mar 25, 2019

fixes #458

@flying-sheep

This comment has been minimized.

Copy link
Member

flying-sheep commented Mar 26, 2019

That’s super redundant now.

Please extract all that text from doc_scatter_bulk into another variable and import and use that one instead.

@fabianrost84

This comment has been minimized.

Copy link
Contributor Author

fabianrost84 commented Mar 26, 2019

In #458 @fidelram suggested that this would be the way to go. If I put the text into another variable, this variable will only be used once. Does this still make sense? Anyways, I think this is just temporary until pl.scatter is in a better shape if I follow @falexwolf correctly.

@flying-sheep

This comment has been minimized.

Copy link
Member

flying-sheep commented Mar 26, 2019

Deduplication always makes sense. I often use speaking variable names instead of comments to clarify what I’m doing.

Here the 6 reasons why I’m convinced of the way I suggested:

1) If I look at your change as it is, I have no idea what parameters are missing compared to doc_scatter_bulk. If you used variables, we could see it at a glance. 2) You could add a comment explaining that this one is just temporary (Hard to do in-line in a docstring).

If one makes changes to the parameters, 3) they only have to make them once and 4) can’t forget to make them twice. 5) Also the diff becomes much smaller and 6) git will be able to track doc changes that are made in the mean time.

So do it please.

@fabianrost84 fabianrost84 force-pushed the fabianrost84:fabianrost84-scatter-doc branch from 10c393a to b693241 Mar 27, 2019

@fabianrost84

This comment has been minimized.

Copy link
Contributor Author

fabianrost84 commented Mar 27, 2019

@flying-sheep what about this then?

@flying-sheep

This comment has been minimized.

Copy link
Member

flying-sheep commented Mar 28, 2019

There’s still duplication. There shouldn’t be any duplicated parameter docs. I’d expect something like this:

The diff will be 8 changed lines or so as opposed to dozens of added duplicate lines.

_doc_scatter_common = '''
sort_order
...
'''

_doc_scatter_panels = '''
ncol
...
'''

_doc_scatter_meta = '''
title
...
'''

doc_scatter_temp = _doc_scatter_common + _doc_scatter_meta
doc_scatter_bulk = _doc_scatter_common + _doc_scatter_panel + _doc_scatter_meta
@fabianrost84

This comment has been minimized.

Copy link
Contributor Author

fabianrost84 commented Mar 28, 2019

OK, I think, now I got you point.

@fabianrost84 fabianrost84 force-pushed the fabianrost84:fabianrost84-scatter-doc branch from b693241 to 40d2cd2 Mar 28, 2019

@fabianrost84

This comment has been minimized.

Copy link
Contributor Author

fabianrost84 commented Mar 28, 2019

@flying-sheep Thanks for your help. Like this, maybe?

@flying-sheep flying-sheep merged commit 9cff5a7 into theislab:master Mar 28, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@flying-sheep

This comment has been minimized.

Copy link
Member

flying-sheep commented Mar 28, 2019

beautiful, thank you

@fabianrost84 fabianrost84 deleted the fabianrost84:fabianrost84-scatter-doc branch Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.