You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/07/18 16:07:18 UTC
[GitHub] [beam] pcoet opened a new pull request, #22316: cleaned up types in standard_coders.ts
pcoet opened a new pull request, #22316:
URL: https://github.com/apache/beam/pull/22316
**Please** add a meaningful description for your change here
Fixed 39 lint issues in standard_coders.ts. All clean now! Also added instructions for doing an install from package-lock.json.
@robertwb
@damccorm
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #22316:
URL: https://github.com/apache/beam/pull/22316#discussion_r924724441
##########
sdks/typescript/src/apache_beam/coders/standard_coders.ts:
##########
@@ -28,52 +28,44 @@ import {
} from "./coders";
import { BytesCoder, InstantCoder } from "./required_coders";
import Long from "long";
-import {
- Window,
- Instant,
- IntervalWindow,
- KV,
- PaneInfo,
- Timing,
- WindowedValue,
-} from "../values";
+import { IntervalWindow } from "../values";
// Historical
export * from "./required_coders";
/**
* @fileoverview Defines all of the Apache Beam standard coders.
*
- * Beyond required coders, standard coders provide a efficient ways of encode
+ * Beyond required coders, standard coders provide an efficient way to encode
* data for communication between the runner and various Beam workers for
* types that commonly cross process boundaries. Though none of these coders
- * are strictly necessary, if encodings are given for these types it is highly
+ * is strictly necessary, if encodings are given for these types it is highly
* advised to use these definitions that are interoperable with runners and
* other SDKs.
*
- * For schema-aware transforms RowCoder, which is a coder for rows of data
- * with a predetermined schema, is also advised.
+ * For the schema-aware transform RowCoder, which is a coder for rows of data
+ * with a predetermined schema, it is also advised.
*
* The formal specifications for these coders can be found in
* model/pipeline/src/main/proto/beam_runner_api.proto
*/
-export class StrUtf8Coder implements Coder<String> {
- static URN: string = "beam:coder:string_utf8:v1";
- type: string = "stringutf8coder";
+export class StrUtf8Coder implements Coder<string> {
+ static URN = "beam:coder:string_utf8:v1";
+ type = "stringutf8coder";
encoder = new TextEncoder();
decoder = new TextDecoder();
- encode(element: String, writer: Writer, context: Context) {
- const encodedElement = this.encoder.encode(element as string);
+ encode(element: string, writer: Writer, context: Context) {
+ const encodedElement = this.encoder.encode(element);
BytesCoder.INSTANCE.encode(encodedElement, writer, context);
}
- decode(reader: Reader, context: Context): String {
+ decode(reader: Reader, context: Context): string {
return this.decoder.decode(BytesCoder.INSTANCE.decode(reader, context));
}
- toProto(pipelineContext: ProtoContext): runnerApi.Coder {
+ toProto(): runnerApi.Coder {
Review Comment:
Oh interesting - I didn't realize that. I'm good with leaving it as is, thanks for the explanation!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pcoet commented on a diff in pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
pcoet commented on code in PR #22316:
URL: https://github.com/apache/beam/pull/22316#discussion_r923915280
##########
sdks/typescript/src/apache_beam/coders/standard_coders.ts:
##########
@@ -28,52 +28,44 @@ import {
} from "./coders";
import { BytesCoder, InstantCoder } from "./required_coders";
import Long from "long";
-import {
- Window,
- Instant,
- IntervalWindow,
- KV,
- PaneInfo,
- Timing,
- WindowedValue,
-} from "../values";
+import { IntervalWindow } from "../values";
// Historical
export * from "./required_coders";
/**
* @fileoverview Defines all of the Apache Beam standard coders.
*
- * Beyond required coders, standard coders provide a efficient ways of encode
+ * Beyond required coders, standard coders provide an efficient way to encode
* data for communication between the runner and various Beam workers for
* types that commonly cross process boundaries. Though none of these coders
- * are strictly necessary, if encodings are given for these types it is highly
+ * is strictly necessary, if encodings are given for these types it is highly
* advised to use these definitions that are interoperable with runners and
* other SDKs.
*
- * For schema-aware transforms RowCoder, which is a coder for rows of data
- * with a predetermined schema, is also advised.
+ * For the schema-aware transform RowCoder, which is a coder for rows of data
+ * with a predetermined schema, it is also advised.
*
* The formal specifications for these coders can be found in
* model/pipeline/src/main/proto/beam_runner_api.proto
*/
-export class StrUtf8Coder implements Coder<String> {
- static URN: string = "beam:coder:string_utf8:v1";
- type: string = "stringutf8coder";
+export class StrUtf8Coder implements Coder<string> {
+ static URN = "beam:coder:string_utf8:v1";
+ type = "stringutf8coder";
encoder = new TextEncoder();
decoder = new TextDecoder();
- encode(element: String, writer: Writer, context: Context) {
- const encodedElement = this.encoder.encode(element as string);
+ encode(element: string, writer: Writer, context: Context) {
+ const encodedElement = this.encoder.encode(element);
BytesCoder.INSTANCE.encode(encodedElement, writer, context);
}
- decode(reader: Reader, context: Context): String {
+ decode(reader: Reader, context: Context): string {
return this.decoder.decode(BytesCoder.INSTANCE.decode(reader, context));
}
- toProto(pipelineContext: ProtoContext): runnerApi.Coder {
+ toProto(): runnerApi.Coder {
Review Comment:
It will work, because `toProto()` is assignable to `toProto(pipelineContext: ProtoContext)` in TypeScript. See [this doc](https://www.typescriptlang.org/docs/handbook/type-compatibility.html#comparing-two-functions) and this example:
```typescript
let x = (a: number) => 0;
let y = (b: number, s: string) => 0;
y = x; // OK
x = y; // Error
```
We could use an underscore (`_pipelineContext: ProtoContext`) for documentation, but it wouldn't make the code more correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm merged pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
damccorm merged PR #22316:
URL: https://github.com/apache/beam/pull/22316
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #22316:
URL: https://github.com/apache/beam/pull/22316#discussion_r923843369
##########
sdks/typescript/README.md:
##########
@@ -175,6 +175,12 @@ Install node.js, and then from within `sdks/typescript`.
npm install
```
+To install without rewriting **package-lock.json**, run:
Review Comment:
I assume this came up because running `npm install` currently updates package-lock.json? I'd probably argue that's things working as intended. Package management is always a nightmare, but by specifying minor version pins in the package.json, I'd expect us to be comfortable with dependency updates to the package-lock.json in an arbitrary pr.
FWIW, right now our tests use `npm install` (not `npm ci`), so if you use `npm ci` in local dev you're building something different than what's getting tested in ci. That's probably just us doing the wrong thing in CI though.
##########
sdks/typescript/src/apache_beam/coders/standard_coders.ts:
##########
@@ -28,52 +28,44 @@ import {
} from "./coders";
import { BytesCoder, InstantCoder } from "./required_coders";
import Long from "long";
-import {
- Window,
- Instant,
- IntervalWindow,
- KV,
- PaneInfo,
- Timing,
- WindowedValue,
-} from "../values";
+import { IntervalWindow } from "../values";
// Historical
export * from "./required_coders";
/**
* @fileoverview Defines all of the Apache Beam standard coders.
*
- * Beyond required coders, standard coders provide a efficient ways of encode
+ * Beyond required coders, standard coders provide an efficient way to encode
* data for communication between the runner and various Beam workers for
* types that commonly cross process boundaries. Though none of these coders
- * are strictly necessary, if encodings are given for these types it is highly
+ * is strictly necessary, if encodings are given for these types it is highly
* advised to use these definitions that are interoperable with runners and
* other SDKs.
*
- * For schema-aware transforms RowCoder, which is a coder for rows of data
- * with a predetermined schema, is also advised.
+ * For the schema-aware transform RowCoder, which is a coder for rows of data
+ * with a predetermined schema, it is also advised.
*
* The formal specifications for these coders can be found in
* model/pipeline/src/main/proto/beam_runner_api.proto
*/
-export class StrUtf8Coder implements Coder<String> {
- static URN: string = "beam:coder:string_utf8:v1";
- type: string = "stringutf8coder";
+export class StrUtf8Coder implements Coder<string> {
+ static URN = "beam:coder:string_utf8:v1";
+ type = "stringutf8coder";
encoder = new TextEncoder();
decoder = new TextDecoder();
- encode(element: String, writer: Writer, context: Context) {
- const encodedElement = this.encoder.encode(element as string);
+ encode(element: string, writer: Writer, context: Context) {
+ const encodedElement = this.encoder.encode(element);
BytesCoder.INSTANCE.encode(encodedElement, writer, context);
}
- decode(reader: Reader, context: Context): String {
+ decode(reader: Reader, context: Context): string {
return this.decoder.decode(BytesCoder.INSTANCE.decode(reader, context));
}
- toProto(pipelineContext: ProtoContext): runnerApi.Coder {
+ toProto(): runnerApi.Coder {
Review Comment:
Will this work? Shouldn't this be implementing https://github.com/apache/beam/blob/23423e23d3560185792894c7e4d7ba0d4ca4092d/sdks/typescript/src/apache_beam/coders/coders.ts#L139
##########
sdks/typescript/src/apache_beam/coders/standard_coders.ts:
##########
@@ -28,52 +28,44 @@ import {
} from "./coders";
import { BytesCoder, InstantCoder } from "./required_coders";
import Long from "long";
-import {
- Window,
- Instant,
- IntervalWindow,
- KV,
- PaneInfo,
- Timing,
- WindowedValue,
-} from "../values";
+import { IntervalWindow } from "../values";
// Historical
export * from "./required_coders";
/**
* @fileoverview Defines all of the Apache Beam standard coders.
*
- * Beyond required coders, standard coders provide a efficient ways of encode
+ * Beyond required coders, standard coders provide an efficient way to encode
* data for communication between the runner and various Beam workers for
* types that commonly cross process boundaries. Though none of these coders
- * are strictly necessary, if encodings are given for these types it is highly
+ * is strictly necessary, if encodings are given for these types it is highly
* advised to use these definitions that are interoperable with runners and
* other SDKs.
*
- * For schema-aware transforms RowCoder, which is a coder for rows of data
- * with a predetermined schema, is also advised.
+ * For the schema-aware transform RowCoder, which is a coder for rows of data
+ * with a predetermined schema, it is also advised.
*
* The formal specifications for these coders can be found in
* model/pipeline/src/main/proto/beam_runner_api.proto
*/
-export class StrUtf8Coder implements Coder<String> {
- static URN: string = "beam:coder:string_utf8:v1";
- type: string = "stringutf8coder";
+export class StrUtf8Coder implements Coder<string> {
+ static URN = "beam:coder:string_utf8:v1";
+ type = "stringutf8coder";
encoder = new TextEncoder();
decoder = new TextDecoder();
- encode(element: String, writer: Writer, context: Context) {
- const encodedElement = this.encoder.encode(element as string);
+ encode(element: string, writer: Writer, context: Context) {
+ const encodedElement = this.encoder.encode(element);
BytesCoder.INSTANCE.encode(encodedElement, writer, context);
}
- decode(reader: Reader, context: Context): String {
+ decode(reader: Reader, context: Context): string {
return this.decoder.decode(BytesCoder.INSTANCE.decode(reader, context));
}
- toProto(pipelineContext: ProtoContext): runnerApi.Coder {
+ toProto(): runnerApi.Coder {
Review Comment:
I have a similar question elsewhere in this file for toProto and other functions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #22316:
URL: https://github.com/apache/beam/pull/22316#discussion_r923846599
##########
sdks/typescript/README.md:
##########
@@ -175,6 +175,12 @@ Install node.js, and then from within `sdks/typescript`.
npm install
```
+To install without rewriting **package-lock.json**, run:
Review Comment:
Filed issue #22322 and opened PR #22323 to address the CI issue.
I do still think we should be using `npm install` for most local dev and including package-lock changes in PRs though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pcoet commented on a diff in pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
pcoet commented on code in PR #22316:
URL: https://github.com/apache/beam/pull/22316#discussion_r923918731
##########
sdks/typescript/src/apache_beam/coders/standard_coders.ts:
##########
@@ -28,52 +28,44 @@ import {
} from "./coders";
import { BytesCoder, InstantCoder } from "./required_coders";
import Long from "long";
-import {
- Window,
- Instant,
- IntervalWindow,
- KV,
- PaneInfo,
- Timing,
- WindowedValue,
-} from "../values";
+import { IntervalWindow } from "../values";
// Historical
export * from "./required_coders";
/**
* @fileoverview Defines all of the Apache Beam standard coders.
*
- * Beyond required coders, standard coders provide a efficient ways of encode
+ * Beyond required coders, standard coders provide an efficient way to encode
* data for communication between the runner and various Beam workers for
* types that commonly cross process boundaries. Though none of these coders
- * are strictly necessary, if encodings are given for these types it is highly
+ * is strictly necessary, if encodings are given for these types it is highly
* advised to use these definitions that are interoperable with runners and
* other SDKs.
*
- * For schema-aware transforms RowCoder, which is a coder for rows of data
- * with a predetermined schema, is also advised.
+ * For the schema-aware transform RowCoder, which is a coder for rows of data
+ * with a predetermined schema, it is also advised.
*
* The formal specifications for these coders can be found in
* model/pipeline/src/main/proto/beam_runner_api.proto
*/
-export class StrUtf8Coder implements Coder<String> {
- static URN: string = "beam:coder:string_utf8:v1";
- type: string = "stringutf8coder";
+export class StrUtf8Coder implements Coder<string> {
+ static URN = "beam:coder:string_utf8:v1";
+ type = "stringutf8coder";
encoder = new TextEncoder();
decoder = new TextDecoder();
- encode(element: String, writer: Writer, context: Context) {
- const encodedElement = this.encoder.encode(element as string);
+ encode(element: string, writer: Writer, context: Context) {
+ const encodedElement = this.encoder.encode(element);
BytesCoder.INSTANCE.encode(encodedElement, writer, context);
}
- decode(reader: Reader, context: Context): String {
+ decode(reader: Reader, context: Context): string {
return this.decoder.decode(BytesCoder.INSTANCE.decode(reader, context));
}
- toProto(pipelineContext: ProtoContext): runnerApi.Coder {
+ toProto(): runnerApi.Coder {
Review Comment:
Let me know if you think that's preferable -- to use `_unusedParam: ParamType`. I can do that instead...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #22316:
URL: https://github.com/apache/beam/pull/22316#discussion_r923846599
##########
sdks/typescript/README.md:
##########
@@ -175,6 +175,12 @@ Install node.js, and then from within `sdks/typescript`.
npm install
```
+To install without rewriting **package-lock.json**, run:
Review Comment:
Filed #22322 to address the CI issue.
I do still think we should be using `npm install` for most local dev and including package-lock changes in PRs though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pcoet commented on a diff in pull request #22316: cleaned up types in standard_coders.ts
Posted by GitBox <gi...@apache.org>.
pcoet commented on code in PR #22316:
URL: https://github.com/apache/beam/pull/22316#discussion_r923915798
##########
sdks/typescript/README.md:
##########
@@ -175,6 +175,12 @@ Install node.js, and then from within `sdks/typescript`.
npm install
```
+To install without rewriting **package-lock.json**, run:
Review Comment:
Ack. I removed the install note.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org