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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parsing methods as well #17

Closed
wants to merge 5 commits into from

Conversation

valscion
Copy link
Collaborator

@valscion valscion commented May 4, 2018

Oh well, I guess I really wanna go head first to tackle #2 馃槃

This PR starts off with a proof of concept to see if I can get any output for method parsing. Turns out there doesn't seem to be much left to do to get some output that is actual working code

@@ -92,7 +92,7 @@ function parseLinks(description/*: Cheerio*/) {
*
* @param {Store} store
*/
async function requestAndParse(store/*: Store*/) {
async function requestAndParse(store/*: Store*/, hacketyhack/*: 'Field' | 'Parameters' */) {
Copy link
Owner

Choose a reason for hiding this comment

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

馃槅

Copy link
Collaborator Author

@valscion valscion May 4, 2018

Choose a reason for hiding this comment

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

Yeah I just tried to get the parser going 馃槃

This was funky to run on the API website in developer tools console:

Array.from(document.body.querySelectorAll('body table'))
  .map(table => [table, table.querySelector(':scope tr:first-child td:first-child')])
  .filter(([_, el]) => el.innerText === 'Parameters')
  .map(([table, el]) => {

let elTry = table;
for (let i = 0; i < 50; i++) {
  elTry = elTry.previousElementSibling;
  if (elTry === null) { return null; }
  if (elTry.nodeName === 'H4') { return elTry; } 
}

}).map(heading => heading.innerText)

I wanted to see if the same approach of finding names would work, and seems like it does 馃槃

@valscion
Copy link
Collaborator Author

valscion commented May 4, 2018

Any ideas on how to progress with this? If you feel like you have ideas, now that you have invited me to contribute I can also push this branch to this repository and let you figure things out as well :)

I'm mostly interested in getting this feature supported and am not looking for glory, so I don't mind if you come up with a better idea 馃槃

@sergeysova
Copy link
Owner

sergeysova commented May 4, 2018

I think, Store and Builders should be refactored:

  • Add collection of types and methods
  • Add support for methods in builders (not for rust)
store.types.get('SomeType')

const method = store.methods.get('methodName')
method.name // methodName
method.arguments // array of { name: 'foo', type: FooType }
method.returns // ReturnType

To parse methods, parser should be improved:

  • Parser should correctly distinguish method from type (first column in types "Field", in methods "Parameters" and column "Required")
  • Correct parsing return type from Returns a UserProfilePhotos object. and Returns true. https://core.telegram.org/bots/api#getuserprofilephotos
  • Correct parsing InlineKeyboardMarkup or ReplyKeyboardMarkup or ReplyKeyboardRemove or ForceReply to union of types
    ....

Other cases maybe covered by exists parser.

This generates working output but this isn't a solid approach, right?
@valscion
Copy link
Collaborator Author

valscion commented May 4, 2018

Yeah. I might get to this tomorrow, it's getting quite late here (its 0:21 past midnight right now) so I'm gonna close my laptop for now.

I managed to hack together even more crappy code to get some types that actually would work together 馃槄

Let's see where this will go later. Good night!

/**
* TODO parse union properly
*/
declare type InlineKeyboardMarkup or ReplyKeyboardMarkup or ReplyKeyboardRemove or ForceReply = InlineKeyboardMarkup | ReplyKeyboardMarkup | ReplyKeyboardRemove | ForceReply;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah my hack didn't touch these declarations at all. Only their references 馃槤

/**
* Yes
*/
chat_id: IntegerOrString,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Such pretty type name. I guess we will really want to have chat_id: number | string here? As Integer is a native type of number and String is a native type of string.

Copy link
Owner

Choose a reason for hiding this comment

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

In this case, I agree

/**
* Optional
*/
reply_markup: InlineKeyboardMarkupOrReplyKeyboardMarkupOrReplyKeyboardRemoveOrForceReply,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And here we will want to have:

reply_markup: InlineKeyboardMarkup | ReplyKeyboardMarkup | ReplyKeyboardRemove | ForceReply

As the description says "Optional", should this then be

reply_markup?: InlineKeyboardMarkup | ReplyKeyboardMarkup | ReplyKeyboardRemove | ForceReply

right?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes

@valscion
Copy link
Collaborator Author

valscion commented May 5, 2018

Closing in favor of #18

@valscion valscion closed this May 5, 2018
@valscion valscion deleted the parse-methods branch May 5, 2018 21:29
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.

None yet

2 participants