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

migrate destinations to use airbyte protocol #557

Merged
merged 2 commits into from
Oct 14, 2020

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Oct 14, 2020

This is mostly just find/replace type of operations.

Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

LGTM once build pas


public class Instants {

public static long millisToSeconds(long millis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can call it toSeconds as long as the argument is called millis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


public class Instants {

public static long toSeconds(long millis) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary when long seconds = TimeUnit.MILLISECONDS.toSeconds(millis); is part of the standard library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough.

@cgardens cgardens force-pushed the cgardens/migrate_destinationst_airbyte_protocol branch from b9fb9c7 to aee8806 Compare October 14, 2020 17:16
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.

4 participants