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

Fix page token usage. #971

Merged
merged 2 commits into from
May 1, 2023
Merged

Conversation

riemanli
Copy link
Contributor

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

@riemanli
Copy link
Contributor Author

Copy link
Contributor

@tristanvuong2021 tristanvuong2021 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @riemanli)

a discussion (no related file):
Since you are changing the package anyways, there should be a page token in the reporting folder. So kingdom and reporting get their own page tokens. Right now, the page token proto has both kingdom and reporting specific messages, but the path is technically the kingdom.


Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)

a discussion (no related file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

Since you are changing the package anyways, there should be a page token in the reporting folder. So kingdom and reporting get their own page tokens. Right now, the page token proto has both kingdom and reporting specific messages, but the path is technically the kingdom.

That's a good point. For that, do I create a page token proto for both v1 and v2 or separate page token protos for each version?


Copy link
Contributor

@tristanvuong2021 tristanvuong2021 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 14 files reviewed, 1 unresolved discussion (waiting on @riemanli)

a discussion (no related file):

Previously, riemanli (Rieman) wrote…

That's a good point. For that, do I create a page token proto for both v1 and v2 or separate page token protos for each version?

I think it is possible to get away with just one.


@riemanli riemanli force-pushed the riemanli_set_expression_compiler branch from 82cb211 to 5c9ddf7 Compare April 27, 2023 22:28
@riemanli riemanli force-pushed the riemanli_fix_java_package_in_cmms_page_token branch from 9fc2f8c to 9ad7a71 Compare April 27, 2023 22:28
Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 20 files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)

a discussion (no related file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

I think it is possible to get away with just one.

I ended up creating two for each version because we changed the naming from reference ID to CMMS ID. PTAL.


@riemanli riemanli force-pushed the riemanli_fix_java_package_in_cmms_page_token branch from 9ad7a71 to 43b9bad Compare April 27, 2023 22:36
@riemanli riemanli changed the title Fix the java package path in the CMMS page token proto. Fix page token usage. Apr 27, 2023
Copy link
Contributor

@tristanvuong2021 tristanvuong2021 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 14 files at r1, 13 of 14 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @riemanli)


src/main/proto/wfa/measurement/api/v2alpha/page_token.proto line 4 at r3 (raw file):

 * Copyright 2021 The Cross-Media Measurement Authors
 *
 * Licensed under the Apache License, Version 2.0 (the "License");

I think the proto files still have the // for the license header


src/main/proto/wfa/measurement/reporting/v1alpha/page_token.proto line 10 at r3 (raw file):

 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software

ditto


src/main/proto/wfa/measurement/reporting/v2alpha/page_token.proto line 10 at r3 (raw file):

 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software

ditto

@riemanli riemanli force-pushed the riemanli_fix_java_package_in_cmms_page_token branch from 43b9bad to b392ddc Compare April 27, 2023 23:26
Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewable status: 17 of 20 files reviewed, 3 unresolved discussions (waiting on @tristanvuong2021)


src/main/proto/wfa/measurement/api/v2alpha/page_token.proto line 4 at r3 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

I think the proto files still have the // for the license header

Ah you're right. Thanks for pointing it out. Fixed.


src/main/proto/wfa/measurement/reporting/v1alpha/page_token.proto line 10 at r3 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

ditto

Done.


src/main/proto/wfa/measurement/reporting/v2alpha/page_token.proto line 10 at r3 (raw file):

Previously, tristanvuong2021 (Tristan Vuong) wrote…

ditto

Done.

Copy link
Contributor

@tristanvuong2021 tristanvuong2021 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @riemanli)

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 14 files at r1, 10 of 14 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier)

@riemanli riemanli force-pushed the riemanli_set_expression_compiler branch from 5c9ddf7 to be04e39 Compare May 1, 2023 16:23
@riemanli riemanli force-pushed the riemanli_fix_java_package_in_cmms_page_token branch from b392ddc to f8b10c3 Compare May 1, 2023 16:23
@riemanli
Copy link
Contributor Author

riemanli commented May 1, 2023

@riemanli queued this pull request to merge with Graphite.

@riemanli riemanli force-pushed the riemanli_set_expression_compiler branch from be04e39 to b8d67e2 Compare May 1, 2023 16:49
@riemanli
Copy link
Contributor Author

riemanli commented May 1, 2023

The Graphite merge of this pull request was cancelled.

@riemanli riemanli force-pushed the riemanli_fix_java_package_in_cmms_page_token branch from f8b10c3 to 9029e21 Compare May 1, 2023 17:02
Base automatically changed from riemanli_set_expression_compiler to main May 1, 2023 17:12
@riemanli riemanli force-pushed the riemanli_fix_java_package_in_cmms_page_token branch from 9029e21 to fbe2bb6 Compare May 1, 2023 17:13
Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 14 files at r1, 10 of 14 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier)

@riemanli riemanli merged commit 02b108e into main May 1, 2023
@riemanli riemanli deleted the riemanli_fix_java_package_in_cmms_page_token branch May 1, 2023 17:46
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