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

adds skipPrettier and self closing #57

Closed
wants to merge 9 commits into from

Conversation

tylerturdenpants
Copy link
Collaborator

@tylerturdenpants tylerturdenpants commented Jun 16, 2019

Disables Prettier completely. An alternative way to format the output of the codemod where in most cases "white space" is preserved. Uses the latest glimmer/syntax to apply selfClosing tags when an element is not in block form.

@GavinJoyce
Copy link
Collaborator

should solve #54 #1 #45

@tylerturdenpants does this solve https://github.com/rajasegar/ember-angle-brackets-codemod/issues/54? I think we need prettier/prettier#6186 for that which I don't believe has been released yet

@GavinJoyce
Copy link
Collaborator

Likewise, does it actually solve the other issues?

As far as I can tell, disabling prettier introduces a bunch of new formatting issues. Eg:

{{admin/about-auto-resizing-textarea-component
    text=editableBiography
    placeholderText='Say hi and help people get to know you by sharing one thing you care about'
    noDataText='Introduce yourself'
    maxLength=maxBiographyLength
    textareaClasses=editableBioTextClasses
    editableMode=editableMode
    error=biographyError
    displayText=model.profile.biography
}}

becomes:

<Admin::AboutAutoResizingTextareaComponent @text={{editableBiography}} @placeholderText="Say hi and help people get to know you by sharing one thing you care about" @noDataText="Introduce yourself" @maxLength={{maxBiographyLength}} @textareaClasses={{editableBioTextClasses}} @editableMode={{editableMode}} @error={{biographyError}} @displayText={{model.profile.biography}} />

@tylerturdenpants
Copy link
Collaborator Author

@GavinJoyce I think it "almost solves" these issues. The main focus was to give the ability to opt-out of prettier. I can rename the title and remove the description.

The other thing is that the PR use glimmer-syntax to correctly print self closing tags.

If this is merged, how should we highlight the changes? Let me know what you think

@GavinJoyce
Copy link
Collaborator

I might be wrong, but this seems to swap one bunch of formatting problems for another. I think it's fine to add a skipPrettier option, but I don't believe that it solves the formatting issues as the output realistically needs to be manually fixed

If this is merged, how should we highlight the changes?

A note in the readme seem fine, were you thinking about doing something else?

@tylerturdenpants
Copy link
Collaborator Author

You're definitely right. Its seems more like a "pick your poison" for formatting. Similarly to what you said, both approaches requires manual fixing. I'll update the README to reflect and submit for your review again.

Thanks!

@tylerturdenpants
Copy link
Collaborator Author

@GavinJoyce I think the option of being able to disable prettier in this PR has some value. I have removed the verbiage of keeping white space. Do we want to merge?

@tylerturdenpants tylerturdenpants changed the title adds skipPrettier, preserves white space, and self closing adds skipPrettier and self closing Jun 24, 2019
```js
{
"helpers": [],
"skipPrettier": true
Copy link
Collaborator

@GavinJoyce GavinJoyce Jun 25, 2019

Choose a reason for hiding this comment

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

Perhaps, instead of a boolean, it would be better to add a formatter option whose possible values can be prettier (the default) and none? That would allow us the option of adding a new formatter (eg. ember-template-recast or prettier-experimental) in future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good. I'll implement that.

@atomkirk
Copy link

Can we merge this? theres quite a few problems with the formatting. I don't understand why these are grouped together anyway? If people want formatting they would just run the formatter…

@tylerturdenpants
Copy link
Collaborator Author

tylerturdenpants commented Aug 27, 2019

Sorry @atomkirk. Skipping Prettier and Closing elements kinda go hand in hand, disabling Prettier wont give you closing tags, so that needed to be addressed as part of the opt in/out A few of the contributors have dropped off, so the man power has been severely reduced. A majority of the effort has gone into #97 which will keep a huge percentage of the original whitespacing in place. I'm 2 failing test away from releasing if you would like to lend me a hand.

@atomkirk
Copy link

cool. I'll try to help. But why is formatting wrapped into this codemod at all? Why not just leave the formatting as-is.

Also, an earlier handlebars lint config that was the default when we upgraded ember told us to remove /> from places and now this adds it back. any idea why?

@tylerturdenpants
Copy link
Collaborator Author

So if you were to run this codemod without the formatters on your templates, the templates would change to such a degree that it most likely would be undesirable. The formatters were an attempt to get templates back to something recognizable.

This is where ember-template-recast comes in. This codemod library helps keep the original formatting in place.

As far as your second question in regards to />, I don't have an answer for that one.

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