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

Add test case for default-initialized example #367

Merged
merged 4 commits into from
Jul 19, 2019

Conversation

lukebjerring
Copy link
Contributor

@lukebjerring lukebjerring commented Jul 18, 2019

From https://heycam.github.io/webidl/#idl-operations

The following IDL fragment defines an interface with an operation that takes a dictionary argument:

dictionary LookupOptions {
  boolean caseSensitive = false;
};

[Exposed=Window]
interface AddressBook {
  boolean hasAddressForName(USVString name, optional LookupOptions options = {});
};
If hasAddressForName is called with only one argument, the second argument will be a default-initialized LookupOptions dictionary, which will cause caseSensitive to be set to false.

Test case currently fails (expected to, as seems to be the cause of w3c/webref#34)

Not intending to land this PR without the fix, but it's a step in the right direction.

default.json content was generated by temporarily omitting the = {} syntax, and JSON.stringify'ing the AST.

@saschanaz
Copy link
Member

Hi, thank you for the PR!

default.json content was generated by temporarily omitting the = {} syntax, and JSON.stringify'ing the AST.

I think this is why the test is failing, could you explain the reason you did it?

And I'm not sure why we need another test as #338 added one with = {}.

@lukebjerring
Copy link
Contributor Author

Well, for one, it's a different context; the test in #338 is a member with default value, not a default value of a parameter to an operator.

The AST fails to generate at all with the = {} present, so I omitted it to generate at least something (to fill the json file). The failure mode for this test is not just a missing default, it's that the whole tree is missing.

Sorry if this reply is sloppy, I'm on my phone :)

@lukebjerring
Copy link
Contributor Author

In case it's not clear in the description:
See w3c/webref#34 for how this is manifesting problematically in a real-world application.

@saschanaz
Copy link
Member

saschanaz commented Jul 19, 2019

Well, for one, it's a different context; the test in #338 is a member with default value, not a default value of a parameter to an operator.

Ah, definitely worth then 👍

The AST fails to generate at all with the = {} present, so I omitted it to generate at least something (to fill the json file). The failure mode for this test is not just a missing default, it's that the whole tree is missing.

What did you do to generate AST? npm run acquire (after running npm run build) should autogenerate the JSON file, and I see it correctly generates an AST as expected. I'll push the result so that you can check it.

@marcoscaceres Do you have an idea why Travis is failing to start again? I thought the migration to service to app fixed it, but seems not 🤔

PS: The wierd g {} problem when doing npm run acquire should be removed by #369.

@marcoscaceres
Copy link
Member

Having a look at Travis...

@marcoscaceres
Copy link
Member

Hmm... I don't have rights to access the settings... but what's more strange is that the other PRs are all fine.

@saschanaz
Copy link
Member

I think Travis has a problem only with PRs from forks.

@marcoscaceres
Copy link
Member

Yeah, I was thinking the same... maybe just merge and hope for the best 🙊😂

@saschanaz saschanaz merged commit 3130c0b into w3c:gh-pages Jul 19, 2019
@lukebjerring
Copy link
Contributor Author

Thanks!

@lukebjerring lukebjerring deleted the default branch July 19, 2019 10:37
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.

3 participants