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

refactor: move syntax grammars to TS definitions #592

Merged
merged 2 commits into from
Jan 29, 2020

Conversation

ayazhafiz
Copy link
Member

This commit refactors remaining existing grammars to use TS definitions
as started in #581.

This commit refactors remaining existing grammars to use TS definitions
as started in angular#581.
@ayazhafiz
Copy link
Member Author

@dannymcgee if you'd like to take a look

Copy link
Contributor

@kyliau kyliau left a comment

Choose a reason for hiding this comment

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

LGTM, but I think the generated syntax files should be checked in along with the source that generates them. (TypeScript repo also does this with their compiled Javascript output)
This would make it

  1. much easier to inspect the compiled syntaxes,
  2. helpful to check the diff with existing file, (there should not be any in this PR, I guess?)
  3. simplify the build process. At build time they just have to the copied to the dist folder.

We just need to make sure the CI system checks the generated files to ensure they are in sync with the source.

What do you think?

@ayazhafiz
Copy link
Member Author

That sounds good. Do you mind if I put that in the next PR? It goes well with putting the generated files in the root syntaxes directory, which makes development easier as well.

@kyliau
Copy link
Contributor

kyliau commented Jan 29, 2020

Yeah separate PR is fine, thank you!

@ayazhafiz ayazhafiz merged commit 564edb8 into angular:master Jan 29, 2020
@ayazhafiz ayazhafiz deleted the e/ts-form branch January 29, 2020 19:42
@dannymcgee
Copy link
Contributor

@dannymcgee if you'd like to take a look

Sorry I'm just seeing this, but looks good to me! The only thing I would note for the next PR is that if we're going to use PascalCase for the grammar identifiers we should probably also rename template to Template.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants