[FCL-533] ⚠️ Introduce methods to select the best identifier in various cases #810
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…when testing required attributes Using `getattr` with a default of `False` will raise an unexpected error when the attribute is present and has a value of `False`. `hasattr` simply checks for its existence, regardless of value.
This allows us to decide which of a range of possible identifiers should be preferred for a document
This is instead of assuming that an NCN will always be present. As a result, this has had knock-on changes in some other places which were previously relying on this incorrect behaviour. This includes some breaking changes to method signatures, which can now return a `None` in places they previously could not. Since relying on the presence of an NCN is now considered dangerous, these methods have been marked as deprecated. BREAKING CHANGE: Methods which were previously guaranteed to return a Neutral Citation may now return `None`.
jacksonj04
force-pushed
the
FCL-533-pui-needs-to-be-able-to-select-the-best-identifier-for-any-document
branch
2 times, most recently
from
7cf2d3c to
eabfe34
Compare
jacksonj04
force-pushed
the
FCL-533-pui-needs-to-be-able-to-select-the-best-identifier-for-any-document
branch
from
eabfe34 to
b204454
Compare
dragon-dxw
reviewed
Dec 18, 2024
| Typically, this will be the neutral citation number, should it exist. | ||
| Should typically be overridden in subclasses. | ||
| """ | ||
| def best_human_identifier(self) -> Optional[Identifier]: |
There was a problem hiding this comment.
Do all identifiers have a sensible __str__ such that this is a good drop-in replacement despite the type change? (It's probably just the value)
There was a problem hiding this comment.
No, but I want this to explode where we're not using it properly. This is definitely a breaking change.
dragon-dxw
reviewed
Dec 18, 2024
dragon-dxw
approved these changes
Dec 18, 2024
|
Could you check whether this causes any typing issues in EUI/PUI? |
|
It definitely does cause typing problems, but they're expected (eg it's places where the apps assume documents always have NCNs). I'm in the process of fixing in EUI (see #1812), and this will be a major version bump. |
dragon-dxw
changed the title
jacksonj04
deleted the
FCL-533-pui-needs-to-be-able-to-select-the-best-identifier-for-any-document
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE
None.Feat
Fix