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 to use Dart 2 core library lowercase constant names. #272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix to use Dart 2 core library lowercase constant names. #272

wants to merge 1 commit into from

Conversation

munificent
Copy link

No description provided.

@adambender
Copy link
Contributor

Hey Bob it looks like this change is failing the build. It looks like it is just a couple unused imports, would you mind fixing that?

@munificent
Copy link
Author

Those unused import hints are cascaded errors because it doesn't realize that jsonDecode() is defined in there. That's because Travis is trying to use an older version of Dart 1.x stable.

I could change the config to use the latest dev channel build but that's going to cause massive failures. The code generation tools in this package are nowhere near being strong mode clean or Dart 2 ready. I spent some time trying to make it strong mode clean but it was a rathole and I gave up. The problem is that it's using Parser in a very dynamic way (basically using Parser<dynamic> everywhere instead of a typed parser) and passing in callbacks that expect typed parameters. That doesn't fly in Dart 2. To fix that, you need to either type all the parsers correctly or loosen all of those callbacks to take dynamic. The latter is gross — it makes the package even less typed than it is now. I tried the former, but it looks like there are a couple of places in the grammar where sometimes it parses a single value and sometimes a list of values. I didn't have enough context to figure out how to type those.

The generated code that is used at runtime is fine in Dart, though, which is what really matters for users of the package.

@adambender
Copy link
Contributor

Appreciate that you took even that much of a look. In truth, none of the current maintainers (none of us were original authors) have enough context to be able to address most of the Dart 2 related issues because as you point out, this package really takes advantage of the dynamism of Dart 1.

Let me circle back with Ben and Oscar and see what we think can be done. Obviously, getting constant fixed should be an easy thing to do, but this PR is bringing a latent issue to the forefront and I wanna make sure we have a plan.

@munificent
Copy link
Author

For what it's worth, I got really close to having the whole thing Dart 2 clean. There were only a couple of grammar productions that were weird. So it should be doable and it's mostly a mechanical effort — adding lots of type arguments to places that currently just say Parser.

@kevmoo
Copy link
Contributor

kevmoo commented Jul 27, 2018

@munificent – just need to switch travis over to use dev instead of stable...or wait until next week 😄

@tonyclickspace
Copy link

Dart 2 landed, this should pass checks now?

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.

4 participants