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

Use ember-template-recast to preserve existing template formatting as much as possible #97

Merged
merged 3 commits into from
Sep 30, 2019

Conversation

tylerturdenpants
Copy link
Collaborator

refactors to ease modification and to support ember template recast a bit better

@rwjblue
Copy link
Member

rwjblue commented Sep 29, 2019

@tylerturdenpants - 👋 how are things going here? Would you have any time to do a quick brain dump on the current status?

@tylerturdenpants
Copy link
Collaborator Author

@rwjblue I said this in st-octane:

So I’m blocked for what I feel is a respectable release using recast for a few reasons.

  1. Nested subexpression with indentation.
    I can ship without this being finished but @rwjblue said we could have an API through recast.
  2. A consensus from contributors or core how to resolve a few issues until I can add
    telemetry.

The issuses mentioned have solutions and are relatively minor.

We can chat more if you like.

@Turbo87 Turbo87 force-pushed the feature/ember-template-recast branch 3 times, most recently from 18cbbf9 to e190d5a Compare September 30, 2019 12:52
@Turbo87 Turbo87 changed the title [WIP] use ember template recast Use ember-template-recast to keep existing template formatting Sep 30, 2019
@Turbo87 Turbo87 added the enhancement New feature or request label Sep 30, 2019
@Turbo87 Turbo87 marked this pull request as ready for review September 30, 2019 12:59
@Turbo87 Turbo87 force-pushed the feature/ember-template-recast branch from e190d5a to 6f9e112 Compare September 30, 2019 13:55
</bg.button>
</BsButtonGroup>"
"
<BsButtonGroup @value={{buttonGroupValue}} @type=\\"checkbox\\" @onChange={{action (mut buttonGroupValue)}} as |bg|>
Copy link
Member

Choose a reason for hiding this comment

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

Can you open an issue specifically about coordinating with ember-template-recast to create an API to allow a transform to customize whitespace for newly created nodes (in the case of this test, the whitespace cannot be inferred because its a fully new ElementNode) then leave an inline comment with a link to that issue?

I'll mention linking to the "new node whitespace preservation issue" in a few other spots as I notice them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This issue ember-template-lint/ember-template-recast#82 has us discussing the above. Can I use this issue for the comment or would you like a new one created?

Copy link
Member

Choose a reason for hiding this comment

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

Ya, that seems great to link to. I mostly wanted to notate inline that we intend for this output to be changed in the future (once the issue has been fixed/released).

@@ -162,31 +163,17 @@ test('deeply-nested-sub', () => {
`;

expect(runTest('deeply-nested-sub.hbs', input)).toMatchInlineSnapshot(`
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment RE: the new node whitespace preservation issue here as well?

@@ -378,34 +363,12 @@ test('link-to-inline', () => {
`;

expect(runTest('link-to-inline.hbs', input)).toMatchInlineSnapshot(`
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment RE: the new node whitespace preservation issue here as well?

<LinkTo @query={{hash direction=\\"desc\\" showArchived=false}}>
Recent Posts
</LinkTo>
<LinkTo @route=\\"apps.app.users.segments.segment\\" @model=\\"all-users\\" @query={{hash searchTerm=searchTerm}}>Users</LinkTo>
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment RE: the new node whitespace preservation issue here as well?

package.json Show resolved Hide resolved
transforms/angle-brackets/transform.js Outdated Show resolved Hide resolved
transforms/angle-brackets/transform.js Outdated Show resolved Hide resolved
transforms/angle-brackets/transform.js Outdated Show resolved Hide resolved
@Turbo87 Turbo87 force-pushed the feature/ember-template-recast branch from 94ac02e to e93e55e Compare September 30, 2019 16:12
@rwjblue rwjblue changed the title Use ember-template-recast to keep existing template formatting Use ember-template-recast to preserve existing template formatting as much as possible Sep 30, 2019
@tylerturdenpants tylerturdenpants merged commit c4d5591 into master Sep 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the feature/ember-template-recast branch September 30, 2019 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants