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: any not a keyword #439

Merged
merged 4 commits into from
Oct 3, 2019
Merged

fix: any not a keyword #439

merged 4 commits into from
Oct 3, 2019

Conversation

TG1999
Copy link
Contributor

@TG1999 TG1999 commented Oct 3, 2019

Signed-off-by: tushar goel [email protected]

This patch closes #436 and includes:

  • A relevant test
  • A relevant documentation update

TG1999 added 2 commits October 3, 2019 14:40
Signed-off-by: tushar goel <[email protected]>
Signed-off-by: tushar goel <[email protected]>
Signed-off-by: tushar goel <[email protected]>
@TG1999
Copy link
Contributor Author

TG1999 commented Oct 3, 2019

Hi @saschanaz I have wrote one test case for any and one for int32 I am opening another issue for writing test cases for solving #251 . Please merge this one so I can open it and start working on it.

Copy link
Member

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

Great work, just some nits 👍

lib/tokeniser.js Outdated
@@ -53,6 +69,7 @@ const nonRegexTerminals = [
"Infinity",
"NaN",
"Promise",
"any",
"async",
Copy link
Member

Choose a reason for hiding this comment

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

Again while you are at it, could you remove async and constructor from this list? They are already in argumentNameKeywords so they are redundant.

lib/tokeniser.js Outdated
@@ -15,6 +15,22 @@ const tokenRe = {
"other": /[^\t\n\r 0-9A-Za-z]/y
};

export const typeNameKeywords=[
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const typeNameKeywords=[
export const typeNameKeywords = [

Make sure the code format be consistent with other lines 😁

@@ -62,7 +62,7 @@ function type_suffix(tokeniser, obj) {
function single_type(tokeniser, typeName) {
let ret = generic_type(tokeniser, typeName) || primitive_type(tokeniser);
if (!ret) {
const base = tokeniser.consume("identifier", ...stringTypes);
const base = tokeniser.consume("identifier", "any", ...stringTypes, ...typeNameKeywords);
Copy link
Member

Choose a reason for hiding this comment

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

This is okay, but we could probably move any into typeNameKeywords 👍

@@ -0,0 +1,3 @@
Syntax error at line 1 in Int32-keyword.webidl:
Copy link
Member

Choose a reason for hiding this comment

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

👍 but could you rename this as int32array-keyword because

  • I would keep filenames in lowercase, just for consistency
  • We may get int32 in the future, so better be more specific

Sorry to be picky here 😱

Signed-off-by: tushar goel <[email protected]>
@TG1999
Copy link
Contributor Author

TG1999 commented Oct 3, 2019

@saschanaz all changes made 😀

Copy link
Member

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

👍👍👍 Thank you for quick response!

@saschanaz
Copy link
Member

If you want to continue your great works, #256 seems to be next easiest issue to fix.

It might be more complex than previous issues, we want to modify parser logic to allow a set of string in extended attributes. (In lib/productions/extended-attribute.js)

Probably we'll want to copy identifiers() function there and make it parse strings.

I can provide more details but it's already time to bed, so if you need more information, you might try reading existing code before I return.

@TG1999
Copy link
Contributor Author

TG1999 commented Oct 5, 2019

Hi @saschanaz I will surely like to work on this

@TG1999
Copy link
Contributor Author

TG1999 commented Oct 5, 2019

I have read the existing code from their I understood

export function identifiers(tokeniser) {
  const ids = list(tokeniser, { parser: Token.parser(tokeniser, "identifier"), listName: "identifier list" });
  if (!ids.length) {
    tokeniser.error("Expected identifiers but none found");
  }
  return ids;
}

I have to implement this functionality In lib/productions/extended-attribute.js so from where I will get the string that I have to parse

@saschanaz
Copy link
Member

I would modify identifiers into tokens and add a new function identifiersOrStrings, all in extended-attribute.js:

function tokens(tokeniser, tokenName) {
  const toks = list(tokeniser, {
    parser: Token.parser(tokeniser, tokenName),
    listName: tokenName + " list"
  });
  if (!toks.length) {
    tokeniser.error(`Expected ${tokenName}s but none found`);
  }
  return toks;
}

function identifiersOrStrings(tokeniser) {
  return tokens(tokeniser, "identifier") || tokens(tokeniser, "string");
}

And replace existing identifiers(tokeniser) into identifiersOrStrings(tokeniser). This will allow ("str1", "str2") where we want.

@TG1999
Copy link
Contributor Author

TG1999 commented Oct 6, 2019

Okay I got it @saschanaz , will be making a PR soon then we can discuss more changes on the PR itself.

@saschanaz
Copy link
Member

Looking forward to the PR! (If you are blocked on anything, I'm always here to help 👍)

@TG1999
Copy link
Contributor Author

TG1999 commented Oct 9, 2019

Hi, right now I am stucked with some college work, will try to get back to you by weekend and thanks for your help 😄

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.

any is not a keyword
2 participants