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

Change code-formatter from ESLint to Prettier #1

Merged
merged 3 commits into from
Sep 10, 2020

Conversation

mikelid-zz
Copy link
Contributor

@mikelid-zz mikelid-zz commented Aug 27, 2020

Description

  • remove ESLint formatting rules and add prettier/@typescript-eslint to ensure not formatting rules are applied.
  • add prettier configuration.
  • add new tasks to package.json & husky pre-commit step.
  • apply prettier to all files.

Motivation and Context

We have been using ESLint as the formatter, however it has some limitations:

  1. it is slow, particularly in vscode, and particularly for formatting concerns.
  2. format errors would appear as lint-errors causing a lot of visual clutter while developing.
  3. our formatting rules were very much 'house rules' which will always been open to debate.

Testing

  • regression tests passing
  • review of formatting changes done by inspection. col=110 was chosen as a best compromise.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs(Add new document content)
  • Translate Docs(Translate document content)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • My commit message follows Conventional Commit Guideline

License

  • I confirm that this pull request can be released under the Apache 2 license

@mikelid-zz mikelid-zz marked this pull request as ready for review August 27, 2020 01:08
@mikelid-zz mikelid-zz requested review from Shreyas-vgr and a team August 27, 2020 01:09
@@ -0,0 +1,3 @@
node_modules
dist
coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: New line required

Should we add this as a style rule ?

.nycrc
.nyc_output
coverage/
coverage/
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Same as previous new line at EOF

]);
});

test('product value set and changing it requires confirmation and value changed act', async () => {
const requestHandler = new ControlHandler(new ListControlManager());
await testE2E(requestHandler, [
'U: iPhone', TestInput.of(SingleValueControlIntent.of('AppleSuite', { AppleSuite: 'iPhone' })),
'U: iPhone',
TestInput.of(SingleValueControlIntent.of('AppleSuite', { AppleSuite: 'iPhone' })),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is this ok? Kinda looks weird where we had individual lines alternative for User and Alexa earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes there are going to be some cases where the preferred format isn't achieved. I see many reports of people eventually saying they don't mind the change and that the opinionated formatter is liberating. I'm a little skeptical, but I'm willing to try. We might find different ways to address it, and worst case we can use

// prettier-ignore

@mikelid-zz mikelid-zz merged commit e6e3475 into develop Sep 10, 2020
@mikelid-zz mikelid-zz deleted the mliddell/prettier branch October 16, 2020 22:19
@mikelid-zz mikelid-zz mentioned this pull request Dec 11, 2020
13 tasks
mikelid-zz pushed a commit that referenced this pull request Feb 4, 2021
- ControlManager.handleInternalError can produce a complete response,
  including setting the shouldEndSession flag.

- ControlHandler has three modes for handling exceptions that occur
  during initialization and/or canHandle.
   - Default - ControlManager.handleInternalError produces response
   - Alternative #1 - silently return canHandle=false
   - Alternative #2 - rethrow exception to topLevel

BREAKING CHANGE:
 - the default behavior for an exception thrown during canHandle is
   changed. Revert to the old behavior by setting
   ControlHandler.canHandleThrowBehavior = 'Rethrow'
mikelid-zz pushed a commit that referenced this pull request Feb 4, 2021
- ControlManager.handleInternalError can produce a complete response,
  including setting the shouldEndSession flag.

- ControlHandler has three modes for handling exceptions that occur
  during initialization and/or canHandle.
   - Default - ControlManager.handleInternalError produces response
   - Alternative #1 - silently return canHandle=false
   - Alternative #2 - rethrow exception to topLevel

BREAKING CHANGE:
 - the default behavior for an exception thrown during canHandle is
   changed. Revert to the old behavior by setting
   ControlHandler.canHandleThrowBehavior = 'Rethrow'
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.

2 participants