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

feat: Looker SDK generator for the Dart language #933

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bryans99
Copy link
Collaborator

@bryans99 bryans99 commented Jan 8, 2022

Description:

A generator for the Dart language. Note, only supports Looker SDK 4.0.

Changes:

  1. Dart generator.
  2. Tests for dart generator.
  3. Generated dart methods and models.
  4. Runtime library for dart.
  5. Example dart implementation.
  6. Dart unit tests,
  7. Dart end to end tests.

Description:

A generator for the Dart language. Note, only supports Looker SDK 4.0.

Changes:

1. Dart generator.
2. Tests for dart generator.
3. Generated dart methods and models.
4. Runtime librart for dart.
5. Example dart implementation.
6. Unit tests,
7. End to end test.
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2022

Codegen Tests

    1 files    18 suites   31s ⏱️
422 tests 408 ✔️ 14 💤 0 ❌
423 runs  409 ✔️ 14 💤 0 ❌

Results for commit 6e70e89.

@bryans99 bryans99 marked this pull request as draft January 10, 2022 01:37
@bryans99
Copy link
Collaborator Author

Converted to draft. google g3 reports a bunch of lint errors. Turned out I had missed turning on full lint. Have figured out how to do that but it will take a while to fix.

@bryans99 bryans99 marked this pull request as ready for review January 10, 2022 03:41
@github-actions
Copy link
Contributor

Codegen Tests

    1 files    18 suites   26s ⏱️
422 tests 408 ✔️ 14 💤 0 ❌
423 runs  409 ✔️ 14 💤 0 ❌

Results for commit 83309aa.

@github-actions
Copy link
Contributor

Codegen Tests

    1 files    18 suites   30s ⏱️
422 tests 408 ✔️ 14 💤 0 ❌
423 runs  409 ✔️ 14 💤 0 ❌

Results for commit 6d68297.

@github-actions
Copy link
Contributor

Codegen Tests

    1 files    18 suites   26s ⏱️
422 tests 408 ✔️ 14 💤 0 ❌
423 runs  409 ✔️ 14 💤 0 ❌

Results for commit 108aeef.

Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Nothing major but some minor tweaks.

dart/looker_sdk/README.md Outdated Show resolved Hide resolved
dart/looker_sdk/README.md Outdated Show resolved Hide resolved

## Developing

Relies on `yarn` and `dart` being installed. This was developed with `dart` version `2.15.1` so the recommendation is to have a version of dart that is at least at that version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add a Dart install link? Is Dart config supported with Nix? Would be good to find out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me do that.

Not sure I want to get into nix setup for Dart. You cannot use the normal Dart install for cloudtops (you have to download the SDK from an internal google site). Need to figure out how to document "google" specific stuff.


### Generate

Run `yarn sdk Gen` from the `{reporoot}`. Note that the SDK generator needs to be built `yarn build`. If changing the generator run 'yarn watch` in a separate window. This command generates two files:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be yarn gen dart?

Suggested change
Run `yarn sdk Gen` from the `{reporoot}`. Note that the SDK generator needs to be built `yarn build`. If changing the generator run 'yarn watch` in a separate window. This command generates two files:
Run `yarn gen dart` from the `{reporoot}`. Note that the SDK generator needs to be built with `yarn build` first. If changing the generator run 'yarn watch` in a separate window. This command generates two files:


### Run format

Run `yarn format` from `{reporoot}/dart/looker_sdk` to format the `dart` files correctly. This should be run if you change any of the run time library `dart` files. The repo CI will run the linter and will fail if the files have not been correctly formatted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be hooked into the reformatter for codegen

Comment on lines +6 to +9
AuthAccessToken.fromResponse(Map map, String contentType)
: _accessToken = map['access_token'],
_tokenType = map['token_type'],
_expiresIn = map['expires_in'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think you'll want the refresh token in case someone wants to implement OAuth flow? Or worry about it then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the OAUTH flow I think. This is really targeting flutter and an client id/secret will not cut it. Need to get an understanding of how mobile does authentication.

dart/looker_sdk/lib/src/rtl/constants.dart Outdated Show resolved Hide resolved
Comment on lines +15 to +20
static LookerSDK getSdk() {
if (_sdk == null) {
throw Exception('SDK not initialized');
}
return _sdk;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handy when we only need 1 API supported

Comment on lines +85 to +102
case HttpMethod.get:
var response = await http.get(fullPath, headers: headers);
return handleResponse(response, responseHandler);
case HttpMethod.post:
var response = await http.post(fullPath, headers: headers, body: body);
return handleResponse(response, responseHandler);
case HttpMethod.patch:
var response = await http.patch(fullPath, headers: headers, body: body);
return handleResponse(response, responseHandler);
case HttpMethod.delete:
var response = await http.delete(fullPath, headers: headers);
return handleResponse(response, responseHandler);
case HttpMethod.put:
var response = await http.put(fullPath, headers: headers, body: body);
return handleResponse(response, responseHandler);
case HttpMethod.head:
throw UnimplementedError('head');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're basing this on what I did for TypeScript, I did it this way because I wasn't clever enough with TypeScript and the request lib to abstract the method and params into a single call

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was based on what you did for typescript and also is original code from way back. This can be improved. Let me do that,

name: looker_sdk
version: 0.0.1
environment:
sdk: '>=2.9.0 <3.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious what this does. Does this mean use 2.9.x or greater but not 3.x?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again very old stuff. My recollection is that this was mainly driven by flutter but I believe your summation is correct.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryans99 would we expect this SDK branch to be the best choice for using looker in flutter mobile apps, or platform calls to swift/kotlin SDKs, or rest api calls, or embedded urls, or just depending on things?

@bryans99 bryans99 marked this pull request as draft January 12, 2022 01:01
@github-actions
Copy link
Contributor

Codegen Tests

    1 files    18 suites   32s ⏱️
422 tests 408 ✔️ 14 💤 0 ❌
423 runs  409 ✔️ 14 💤 0 ❌

Results for commit 9f0a8f4.

@drstrangelooker drstrangelooker force-pushed the main branch 4 times, most recently from 9474788 to 5f9930c Compare February 4, 2022 02:33
@neiljaywarner
Copy link

@bryans99 i'm interested in this if you have time to get back to it...

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.

3 participants