-
Notifications
You must be signed in to change notification settings - Fork 481
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
automate DTP's injection of auto-converters so extensions don't have to do this #1065
Labels
Comments
jzacsh
added a commit
to jzacsh/dtp
that referenced
this issue
May 11, 2022
jzacsh
added a commit
that referenced
this issue
Jun 22, 2022
…factorings changes (#1062) * fixes #1058: add MediaContainerResourceTest & more adds MediaContainerResourceTest and fixes bug said test newly reveals in automatic root album creation logic during transmogrification. nit: in nit-fixing doc bug (documentation was wrong on `MediaContainerResource#ensureRootAlbum`) also just lifts conditional logic being pushed down into helper function; taking liberty here with my opinion - that Single Responsibility (SOLID) would remove this complexity to begin with (`ensureRootAlbum` should not care about what a transmogrification is or what its config is). * noop(nit:docs): point TODO at a trackable issue number * noop(readability) run Goog style via clang-format * noop(new api) Media* APIs: generate lowlevel types adds MediaContainerResource (and accompanying `MediaAlbum` under the hood) static APIs that admit these classes just wrap or maintain 1:1 correspondence to Photo* types. Unfortunately only adds test coverage via the top-level usage (`MediaContainerResourceTest`) because there's no unit test coverage in place for MediaAlbum and I'm already spinning my wheels a bit adding missing test infra. But splitting some of the MediaAlbum==PhotoAlbum comparison unit test sanity-checking logic (into non-extant MediaAlbumTest.java) would be the correct thing to do here. motivation: this unblocks continued `MediaVertical` work without the need to have (for example) a `AcmecoPhotoExporter` manually forked copy-pasta style into `AcmecoMediaExporter` and then cause immediate techdebt for us (through code drift). longer-term: This change _does_ bring into question some of the intermediate type planning we're doing here, but we've determined this change is not going to be throw away work (eg: to unblock continued Media vertical expansion) while we revisit and discuss intermediate types a bit more. Those discussions will change _how_ this code is called but are very unlikely to _remove_ callers to this code. * WIP: experimental (not even compiling) draft of converter logic with some TODOs * WIP: nitfixes; cleanups & renames renames variables taht were backwards cleans up code since MediaContainer APIs[^1] were pulled out of this work stream to get committed in parallel. [^1]: commit 8963426 (of branch `kate-jon-pair-utils`) * WIP: fixes compilation errors with my generics setup * noop: revert accidental formatting during merge * noop(refactor) moves container copy logic into ExportResult; unfortunately there are no unit tests of this class * noop(refactor) clang-format & add javadoc * noop(refactor) pull ExportResult<...> construction out * noop(docs) point to a tracked TODO: #1065 * WIP: merge fix: bad merge kept both versions of the same code * adds Media* importer by conversion of Photo* one also some nitpicks on some javadoc. * fix: 2c2c4a3 = uncompiling copy-pasta * fix incorrect marking of Import auto-support for Media Co-authored-by: William Morland <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
for context, see the TODO on lines of
extensions/data-transfer/portability-data-transfer-microsoft/src/main/java/org/datatransferproject/transfer/microsoft/MicrosoftTransferExtension.java
in https://github.com/google/data-transfer-project/pull/1062/files inlined below:The text was updated successfully, but these errors were encountered: