Closed mickael-menu closed 7 months ago
๐ฏ Main theme: Refactoring HREF normalization and models
๐ PR summary: This PR refactors the HREF normalization and models in the codebase. It introduces new URL types (RelativeURL
, AbsoluteURL
, HTTPURL
and FileURL
) and modifies the href
in Link
and Locator
to be normalized as valid URLs. It also includes changes to the tests to reflect these modifications.
๐ Type of PR: Refactoring
๐งช Relevant tests added: True
โฑ๏ธ Estimated effort to review [1-5]: 4, because the PR includes a significant amount of changes across multiple files, including core components like URL handling and href normalization. The changes are not trivial and require a good understanding of the existing codebase to review effectively.
๐ Security concerns: No security concerns found
๐ก General suggestions: The changes in this PR are quite extensive and touch on core components of the codebase. It would be beneficial to ensure that these changes are thoroughly tested, not just through unit tests but also through integration tests and manual testing, to ensure that they do not introduce any regressions.
๐ค Code feedback:
relevant file: Tests/SharedTests/Publication/ManifestTests.swift
suggestion: Ensure that the changes to the hrefs in the tests reflect the intended behavior of the new URL types and href normalization. [important]
relevant line: ["href": "manifest.json", "rel": "self"],
relevant file: Sources/Navigator/PDF/PDFNavigatorViewController.swift
suggestion: It would be beneficial to add error handling for the case where the server fails to remove the endpoint. This could prevent potential issues down the line. [important]
relevant line: try? server?.remove(at: endpoint)
relevant file: Sources/Navigator/PDF/PDFNavigatorViewController.swift
suggestion: It might be beneficial to add a comment explaining why the setupPDFView
method is now unavailable and how the PDFNavigatorDelegate
should be used instead. This could help future developers understand the changes. [medium]
relevant line: @available(*, unavailable, message: "Override PDFNavigatorDelegate
instead")
relevant file: Sources/Navigator/PDF/PDFNavigatorViewController.swift
suggestion: Consider adding a comment explaining the reasoning behind the isPDFFile
check. This could help future developers understand the code. [medium]
relevant line: private lazy var isPDFFile: Bool =
Instructions
Tag me in a comment '@CodiumAI-Agent' and add one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask \<QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs: Generate docstring for new components introduced in the PR. /generate_labels: Generate labels for the PR based on the PR's contents. see the tools guide for more details.
To edit any configuration parameter from the configuration.toml, add --config_path=new_value. For example: /review --pr_reviewer.extra_instructions="focus on the file: ..." To list the possible configuration parameters, add a /config comment.
Changelog
Changed
RelativeURL
,AbsoluteURL
,HTTPURL
andFileURL
). This is helpful because:download(url: HTTPURL) -> FileURL
.Shared
Link
andLocator
'shref
are normalized as valid URLs to improve interoperability with the Readium Web toolkits.self
URL of a manifest anymore. However, you can still normalize the HREFs yourselves by callingManifest.normalizeHREFsToSelf()
.Publication.localizedTitle
is now optional, as we cannot guarantee a publication will always have a title (e.g. a CBZ).Migration guide
Migration of HREFs and Locators (bookmarks, annotations, etc.)
:warning: This requires a database migration in your application, if you were persisting
Locator
objects.In Readium v2.x, a
Link
orLocator
'shref
could be either:a valid absolute URL for a streamed publication, e.g.
https://domain.com/isbn/dir/my%20chapter.html
,a percent-decoded path for a local archive such as an EPUB, e.g.
/dir/my chapter.html
./
).To improve the interoperability with other Readium toolkits (in particular the Readium Web Toolkits, which only work in a streaming context) Readium v3 now generates and expects valid URLs for
Locator
andLink
'shref
.https://domain.com/isbn/dir/my%20chapter.html
is left unchanged, as it was already a valid URL./dir/my chapter.html
becomes the relative URL pathdir/my%20chapter.html
/
prefix to avoid issues when resolving to a base URL.You must migrate the HREFs or Locators stored in your database when upgrading to Readium 3. To assist you, two helpers are provided:
AnyURL(legacyHREF:)
andLocator(legacyJSONString:)
.Here's an example of a GRDB migration that can serve as inspiration:
Kotlin PR: https://github.com/readium/kotlin-toolkit/pull/387