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

automate DTP's injection of auto-converters so extensions don't have to do this #1065

Open
jzacsh opened this issue Apr 21, 2022 · 0 comments
Labels

Comments

@jzacsh
Copy link
Collaborator

jzacsh commented Apr 21, 2022

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:

  // TODO(jzacsh,wmorland) don't keep adding here - just have the converters invoked automatically
  // when Media isn't supported on one or the other side of this equation; this is just a WIP
  // prototype to show the concept of converters at play.
  private static final ImmutableList<String> SUPPORTED_IMPORT_SERVICES =
      ImmutableList.of(CALENDAR, CONTACTS, PHOTOS, MEDIA);
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
Labels
Projects
None yet
Development

No branches or pull requests

1 participant