Skip to content

Conversation

@bertsky
Copy link
Owner

@bertsky bertsky commented Jul 10, 2025

builds on OCR-D#1334

@bertsky bertsky requested review from MehmedGIT and kba July 10, 2025 17:21
@bertsky
Copy link
Owner Author

bertsky commented Jul 10, 2025

Note: I did not touch ocrd/resource_manager.py because I did not want to create too many clashes with OCR-D#1319.

Also, I did only cosmetic changes – but nothing to address too complex functions, or deliberate violations like todo comments. Flake8 is quite inflexible compared to pylint, and the maintainer fends off feature requests like block-level noqa by closing issues and shutting down discussion...

Copy link
Collaborator

@MehmedGIT MehmedGIT left a comment

Choose a reason for hiding this comment

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

I agree with the changes, and I don't have much to add other than suggesting some improvements that should still be accepted by flake8.

Comment on lines +26 to +27
@click.option('-n', '--name', envvar='OCRD_TOOL_NAME', default='log_cli', metavar='LOGGER_NAME',
help='Name of the logger', show_default=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@click.option('-n', '--name', envvar='OCRD_TOOL_NAME', default='log_cli', metavar='LOGGER_NAME',
help='Name of the logger', show_default=True)
@click.option(
'-n', '--name', envvar='OCRD_TOOL_NAME', default='log_cli', metavar='LOGGER_NAME', help='Name of the logger', show_default=True)

How about that convention on all click options? That should still be acceptable from the POV of the linter. For click commands with more parameters, there will be less leading whitespace on each line. Same for all occurrences.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Shouldn't the follow-up line still be indented 4 spaces, though?

IMO these 13 characters are not much of a saving to warrant this pattern in general – most of our lines will be much longer, anyway. I would still favour breaking only when needed, and aligning indentation to the opening parentheses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the follow-up line still be indented 4 spaces, though?

Yes.

Comment on lines +7 to +8
def _poll_endpoint_status(ps_server_host: str, job_id: str, job_type: str, tries: int, wait: int,
print_state: bool = False) -> JobState:
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the ocrd_network module, generally, I tried to respect the following structures.

def method(
    parameter A, parameter B, parameter C, ... ,
    parameter X, ...
) -> return_type

or for cases where each parameter may be long when it contains typing, and a default value.

def method(
    parameter A,
    parameter B: int = DefaultGenerator.getDefault(),
    ...
) -> return_type

Again, for the sake of avoiding many leading whitespaces when the method name is somewhat longer. E.g.,

def very_very_very_very_very_very_very_very_long(parameter A, parameter B, parameter C, ... ,
                                                 parameter X, parameter Y, ... ) -> return type

Also more convenient when reviewing code from the web interface of GitHub. If you find that reasonable as well, we may adopt that for the entire repository.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Agreed – for methods and functions, with type annotations taking up so much space nowadays, I am in favour of using your style throughout. (My changes just attempted to get rid of the errors/warnings ad hoc.)

Copy link
Owner Author

Choose a reason for hiding this comment

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

def method(
    parameter A, parameter B, parameter C, ... ,
    parameter X, ...
) -> 

Flake8 mocks this BTW:

E125 continuation line with same indent as next logical line

I guess it must be 8 spaces after the parentheses of the definition.

@bertsky bertsky merged commit d11346a into ocrd-network-ps-fix-compact-page-range Jul 15, 2025
1 check passed
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