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

Improve and add docstring #14

Open
wants to merge 21 commits into
base: processing-options-class
Choose a base branch
from

Conversation

lguerard
Copy link
Contributor

@lguerard lguerard commented Aug 7, 2024

No description provided.

Copy link
Member

@ehrenfeu ehrenfeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lguerard please see my requests in the PR.

(Sorry, I wasn't aware that github needs this review to be submitted before you actually can see my requests - that's not super well visible when doing it the first time...)

Comment on lines +230 to +232
def process_illumination(
self, value, range_end=None
): # def illumination_select(self, value):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs some attention: the # def illumination_select(self, value): was added when I renamed the function to make it easier to check for required changes in existing scripts (that might have used the function under its original name) to find the correct new function name by simply using a string-search on the code.

Currently this triggers the formatter to break up the function definition statement into three lines, which is rather pointless.

One possible / reasonable option is to add a Notes section to the docstring e.g. like this:

def process_illumination(self, value, range_end=None):
    """Blah docstring title.

    [...]

    Notes
    -------
    Previous function name: illumination_select()
    """


Parameters
----------
value : int or int-like
value : str, int, list of int or list of str
Contains the list of input dimensions, the first input dimension of a range or a single channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long

Please don't write what the parameter contains if used correctly, rather look at the phrasing from the perspective of someone not knowing the method and trying to figure out what should go in there, e.g.

"The illuminations to use for processing, either a single value or a list."

value : str, int, list of int or list of str
Contains the list of input dimensions, the first input dimension of a range or a single channel
range_end : int, optional
Contains the end of the range, by default None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full stop at the end.

self._illumination_processing_option = processing_option
self._illumination_select = dimension_select

def process_tile(self, value, range_end=None): # def tile_select(self, value):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for L232 (docstring notes section)

Comment on lines +278 to +280
def process_timepoint(
self, value, range_end=None
): # def timepoint_select(self, value):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for L232 (docstring notes section)


Parameters
----------
value : int or int-like
value : str, int, list of int or list of str
Contains the list of input dimensions, the first input dimension of a range or a single channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long, see comment for L241


Parameters
----------
value : int or int-like
value : str, int, list of int or list of str
Contains the list of input dimensions, the first input dimension of a range or a single channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long, see comment for L241


Parameters
----------
value : int or int-like
value : str, int, list of int or list of str
Contains the list of input dimensions, the first input dimension of a range or a single channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long, see comment for L241

Parameters
----------
value : str, int, list of int or list of str
Contains the list of input dimensions, the first input dimension of a range or a single channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long, see comment for L241

selection : str
"single", "multiple", or "range"
value : str, int, list of int or list of str
Contains the list of input dimensions, the first input dimension of a range or a single channel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long, see comment for L241

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.

3 participants