Support parsing methods, output for Flow #18

valscion · 2018-05-05T21:07:32Z

This PR replaces #17. I re-wrote the method support after applying what I learned from there.

This PR adds methods to the generated Flow types while leaving a nifty slot to apply support for TypeScript and Rust.

It's easier to later only tweak the builder methods if they've worked like interfaces have before.

My knowledge on how these should be done is limited

Supporting Rust for union argument types will require some work, which I am not able to do now.

valscion · 2018-05-05T21:14:31Z

Currently this PR outputs methods as functions for Flow and as interfaces for TypeScript.

I had to skip Rust types as I wasn't able to figure out how the union argument types should be done. This skipping makes this PR safe to merge (nothing should be broken).

Do you think it would be possible to later do TypeScript formatting with function types? We could also skip outputting methods for TypeScript in this PR so that nothing strange would get in the output, if that is better?

valscion · 2018-05-05T21:32:11Z

Do you think it would be possible to later do TypeScript formatting with function types? We could also skip outputting methods for TypeScript in this PR so that nothing strange would get in the output, if that is better?

As it is so easy to skip TypeScript in this PR for now, I'm going to do it. If at all possible, I'd want to avoid having to tackle it while I also tackled all the other stuff I had to do to get this to work.

valscion · 2018-05-05T21:36:20Z

I think this PR is ready for reviewing now ☺️

sergeysova · 2018-05-06T23:22:05Z

To format typescript we can use tslint, and eslint for flowtypes.

sergeysova · 2018-05-06T23:23:31Z

lib/main.js

@@ -45,6 +49,36 @@ function addBuiltins(store) {
    'InputVenueMessageContent',
    'InputContactMessageContent',
  ]))
+  store.add(new Union('InputMedia', {


What about automaticall add unions to store (after parsing doc page)?

There wasn't many unions on the page currently, so I figured it would be quicker to get started by defining the remaining two unions. I wanted to get to the feature that provides most value quickly, which is outputting types for methods.

It is definitely possible to automatically add unions, and get rid of these three store.add(new Union(... lines. However, that could be done outside of this pull request. Would you be OK with that?

valscion · 2018-05-07T07:01:14Z

eslint for flowtypes

I think prettier would be a better formatting option for flowtypes. Would it be OK to you if I'd setup prettier for this repository? Or if not the entire repository, maybe only for the flow type declarations?

sergeysova · 2018-05-07T11:45:55Z

For all my repositories I use eslint with @atomix/eslint

For flow we can use https://github.com/gajus/eslint-plugin-flowtype

valscion · 2018-05-07T12:01:00Z

Yeah sure ☺️

I think formatting flow and typescript is out of scope of this PR, though

Piterden · 2018-05-07T12:06:21Z

Formatting is always paramount!!!

I mean, working code is useful for certain tasks, but the formatted working code is also useful for learning.

valscion · 2018-05-07T12:16:18Z

Is there something specific in the output that would require formatting in your opinion? Or is there something in my code that would need to be reformatted? I am confused.

Piterden · 2018-05-07T12:24:36Z

lib/parser.js

+    const fieldName = flName.text().trim()
+    let fieldType = flType.text().trim()
+
+    if (fieldName === 'Field' || fieldName === 'Parameters') {


I'd use includes here...

Piterden · 2018-05-07T12:27:02Z

lib/parser.js

+      fieldType = fieldType.split(' or ')
+    }
+
+    if (Array.isArray(fieldType)) {


debug(`${name.text().trim()}.${fieldName}: ${Array.isArray(fieldType) ? fieldType.join(' | ') : fieldType} ::: ${fieldDescription}`)

Piterden · 2018-05-07T12:29:30Z

lib/store.js

+class Method extends Type {
+  /*:: fields: FieldMap*/
+
+  constructor(name/*: string*/, attributes/*: Attributes*/, fields/*: FieldMap*/ = {}) {


I'd write new lines here...

If this repository had prettier installed, we wouldn't have to care 😉. The Union and Interface constructors also have these kind of long lines and the linter does not complain, so I'd like to keep it like this. It looks similar to what the other constructors look like:

Ya, I know ))) Just I like the old-school 80 lettered rule.

Piterden · 2018-05-07T12:33:55Z

lib/store.js

@@ -113,7 +125,7 @@ class Store {
  serialize() {
    return [...this.types.values()]
      .reduce((acc, type) => {
-        if (type instanceof Interface) {
+        if (type instanceof Interface || type instanceof Method) {
          acc.types[type.name] = type
        }
        else if (type instanceof Union) {


I think else is unnecessary here, but it is not this PR...

valscion · 2018-05-07T13:15:21Z

Is this good to go?

sergeysova · 2018-05-08T17:51:17Z

I think current flow declaration for methods is not usable. Ex.:

import { sendChatAction } from 'telegram-typings'

// But I can't use it, because our library doesn't have implementation for function

I propose that API (concept):

import type { SendChatActionPayload, SendChatActionResponse } from 'telegram-typings'

// and declare implementation

function sendChatAction(action: SendChatActionPayload): Promise<SendChatActionResponse> {
  // Send request
}

For example:

https://core.telegram.org/bots/api#getuserprofilephotos

Parse parameters to *Payload type:

type GetUserProfilePhotosPayload = {
  user_id: number,
  offset?: number,
  limit?: number,
}

Parse Returns object to *Response type:

type GetUserProfilePhotosResponse = UserProfilePhotos
// In some cases just replace *Response with target type

In that case telegram-typings library user can implement its functions and methods

sergeysova · 2018-05-08T19:19:40Z

Please, do not make any changes to this branch, just think about my proposal.

sergeysova · 2018-05-08T19:19:53Z

I want to refactor builders, from classes to functions

valscion · 2018-05-08T20:48:04Z

Nice, that would make a lot of sense! Would you be OK if we could get this in with the output stubbed out, i.e. the flow output would be similar to typescript? This way we could get the code responsible for parsing functions in to the master branch and could iterate on proper output format in smaller pull requests ☺

I accidentally caused a bug in f42d873 when doing #18 which caused type generation to be broken

valscion added 12 commits May 5, 2018 21:38

feat: Add missing union types manually

90b7d82

feat: Add InputFile manually

c42fa60

chore: Regenerate types

972e432

feat: Add raw support for parsing union types

fa7cb8b

feat: Parse methods as interfaces

edb35b0

chore: Regenerate types

5514799

chore: Add Method type, copy behavior from Interface

14efd94

It's easier to later only tweak the builder methods if they've worked like interfaces have before.

feat: Write out methods for Flow using function declaration types

1fb3520

chore: Add TODO comments for rust+typescript buildMethod

143f72d

My knowledge on how these should be done is limited

chore: Regenerate types

e833f96

fix: Output correct Rust types by skipping method serialization for now

c0f87e5

Supporting Rust for union argument types will require some work, which I am not able to do now.

chore: Regenerate Rust types

7eeadcd

valscion added 2 commits May 6, 2018 00:28

chore: Refactor interface and method parsing to distinct functions

c481117

chore: Regenerate types

023dd6d

valscion mentioned this pull request May 5, 2018

Support parsing methods as well #17

Closed

valscion added 2 commits May 6, 2018 00:33

chore: Skip TypeScript method support for now

dfbcfaf

chore: Regenerate types

b92b3af

valscion changed the title ~~Support parsing methods, this time for real~~ Support parsing methods, output for Flow May 5, 2018

sergeysova reviewed May 6, 2018

View reviewed changes

Piterden reviewed May 7, 2018

View reviewed changes

valscion force-pushed the parse-methods-try-2 branch from 5864fc5 to a713d93 Compare May 7, 2018 13:10

chore: Apply review comments

f42d873

valscion force-pushed the parse-methods-try-2 branch from a713d93 to f42d873 Compare May 7, 2018 13:11

sergeysova approved these changes May 8, 2018

View reviewed changes

sergeysova merged commit a056f9c into master May 8, 2018

valscion deleted the parse-methods-try-2 branch May 10, 2018 16:00

valscion mentioned this pull request May 10, 2018

Parse methods #2

Closed

valscion added a commit that referenced this pull request May 10, 2018

fix: Bug in parser, breaking type generation

ef317f1

I accidentally caused a bug in f42d873 when doing #18 which caused type generation to be broken

This was referenced May 10, 2018

fix: Bug in parser, breaking type generation #19

Merged

feat: Write method payloads as type aliases for Flowtype #20

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support parsing methods, output for Flow #18

Support parsing methods, output for Flow #18

valscion commented May 5, 2018 •

edited

valscion commented May 5, 2018

valscion commented May 5, 2018

valscion commented May 5, 2018

sergeysova commented May 6, 2018

sergeysova May 6, 2018

valscion May 7, 2018

sergeysova May 7, 2018

valscion commented May 7, 2018

sergeysova commented May 7, 2018 •

edited

valscion commented May 7, 2018

Piterden commented May 7, 2018 •

edited

valscion commented May 7, 2018

Piterden May 7, 2018

Piterden May 7, 2018

Piterden May 7, 2018

valscion May 7, 2018

Piterden May 7, 2018

Piterden May 7, 2018

valscion commented May 7, 2018

sergeysova commented May 8, 2018

sergeysova commented May 8, 2018

sergeysova commented May 8, 2018

valscion commented May 8, 2018

Support parsing methods, output for Flow #18

Support parsing methods, output for Flow #18

Conversation

valscion commented May 5, 2018 • edited

valscion commented May 5, 2018

valscion commented May 5, 2018

valscion commented May 5, 2018

sergeysova commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valscion commented May 7, 2018

sergeysova commented May 7, 2018 • edited

valscion commented May 7, 2018

Piterden commented May 7, 2018 • edited

valscion commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valscion commented May 7, 2018

sergeysova commented May 8, 2018

sergeysova commented May 8, 2018

sergeysova commented May 8, 2018

valscion commented May 8, 2018

valscion commented May 5, 2018 •

edited

sergeysova commented May 7, 2018 •

edited

Piterden commented May 7, 2018 •

edited