Skip to content

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

Merged
flying-sheep merged 2 commits intoscverse:masterfrom
fbnrst:fabianrost84-scatter-doc
Mar 28, 2019
Merged

DOC: remove not accepted arguments from pl.scatter#557
flying-sheep merged 2 commits intoscverse:masterfrom
fbnrst:fabianrost84-scatter-doc

Conversation

@fbnrst
Copy link
Copy Markdown
Contributor

@fbnrst fbnrst commented Mar 25, 2019

fixes #458

@flying-sheep
Copy link
Copy Markdown
Member

That’s super redundant now.

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

@fbnrst
Copy link
Copy Markdown
Contributor Author

fbnrst 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
Copy link
Copy Markdown
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.

@fbnrst
Copy link
Copy Markdown
Contributor Author

fbnrst commented Mar 27, 2019

@flying-sheep what about this then?

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

@fbnrst
Copy link
Copy Markdown
Contributor Author

fbnrst commented Mar 28, 2019

OK, I think, now I got you point.

@fbnrst
Copy link
Copy Markdown
Contributor Author

fbnrst commented Mar 28, 2019

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

@flying-sheep flying-sheep merged commit 9cff5a7 into scverse:master Mar 28, 2019
@flying-sheep
Copy link
Copy Markdown
Member

beautiful, thank you

@fbnrst fbnrst deleted the fabianrost84-scatter-doc branch March 29, 2019 11:09
awnimo pushed a commit to dpeerlab/scanpy that referenced this pull request Dec 17, 2019
Xparx pushed a commit to Xparx/scanpy that referenced this pull request Jan 2, 2020
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.

Documentation for pl.scatter

2 participants