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

make OcrdFile.local_filename str again #1167

Closed
wants to merge 1 commit into from

Conversation

bertsky
Copy link
Collaborator

@bertsky bertsky commented Jan 19, 2024

Fixes a regression in #1079 which changed .local_filename type from str to Path

see https://github.com/OCR-D/core/pull/1079/files#r1459238204

@bertsky
Copy link
Collaborator Author

bertsky commented Jan 19, 2024

Not a good idea – too many related changes. Better directly revert 58bb31b

@kba
Copy link
Member

kba commented Jan 19, 2024

I tried reverting and starting to fix test errors but I think this is not worth the effort.

True, we should have communicated this more clearly in the changelog.

But I think the benefit of reverting this is not worth the effort of potentially breaking already-adapted code.

So unless there is no workaround, I would prefer to retain isinstance(local_filename, Path).

@bertsky
Copy link
Collaborator Author

bertsky commented Jan 24, 2024

But I think the benefit of reverting this is not worth the effort of potentially breaking already-adapted code.

So unless there is no workaround, I would prefer to retain isinstance(local_filename, Path).

Well, ... here's my arguments in favour of reverting to str:

  • it's called local_filename, not local_path
  • it is set via str, not via Path, so why should one not expect to get a str back?
  • local_filename is also used as field/parameter on the CLI (which necessarily means str)
  • I'm pretty sure it's easier to fix in core than in a multitude of external code bases (and we can never be sure we did not miss one, think Eynollah).
  • Already adapted (external) code can easily be identified: search for local_filename. and change there. That's way more specific than the string local_filename in the other direction (adapting to new). Any str(*.local_filename) can be kept, because str(str(.)) == str(.).

@kba
Copy link
Member

kba commented Jan 25, 2024

I'm pretty sure it's easier to fix in core than in a multitude of external code bases

OK, then I will do the revert. And add typing information to avoid such a situation in the future.

kba added a commit that referenced this pull request Feb 6, 2024
@kba
Copy link
Member

kba commented Feb 6, 2024

Superseded by #1182

@kba kba closed this Feb 6, 2024
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