Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

new option: "Silent error on save" #46

Merged
merged 1 commit into from
Feb 14, 2017

Conversation

oriSomething
Copy link
Contributor

Continuation of #45

Please review the code. I'm sure you might want to solve this problem differently

package.json Outdated
@@ -58,47 +58,54 @@
"default": false,
"order": 1
},
"formatOnSaveSilentError": {
"title": "On format on Save, Don't show error message",
"description": "Don't show error if accour on format on save",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo? accour

package.json Outdated
@@ -58,47 +58,54 @@
"default": false,
"order": 1
},
"formatOnSaveSilentError": {
"title": "On format on Save, Don't show error message",
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's some inconsistent capitalization going on here

lib/helpers.js Outdated

format(null, {
ignoreSelection: true,
ignoreErrors: !formatOnSaveSilentError,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just inline the call to getConfigOption, or at least name the variablje ignoreErrors so you can just use property shorthand

editor.getSelectedBufferRanges().forEach(range => {
transformRange(editor, range);
editor.getSelectedBufferRanges().forEach((range) => {
transformRange(editor, range, { ignoreErrors: options.ignoreErrors });
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should just pass options instead of redefining a new object every time like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the options of format contains options which aren't related to transformRange. I prefer to pass to a function only properties that are relevant to that function

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok fair enough

});
} else {
transformRange(editor, bufferRange);
transformRange(editor, bufferRange, { ignoreErrors: options.ignoreErrors });
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should just pass options instead of redefining a new object every time like this

const range = new iter.range.constructor(startModified, end);
transformRange(editor, range);
transformRange(editor, range, { ignoreErrors: options.ignoreErrors });
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should just pass options instead of redefining a new object every time like this

lib/helpers.js Outdated
@@ -24,22 +21,24 @@ const getFormatOptions = () => PRETTIER_CORE_OPTIONS.reduce(setConfigOption, {})

const getCurrentScope = () => atom.workspace.getActiveTextEditor().getGrammar().scopeName;

const executePrettier = (text, formatOptions) => {
const executePrettier = (text, formatOptions, options = { ignoreErrors: false }) => {
try {
return prettier.format(text, formatOptions);
} catch (e) {
const message = `prettier-atom: ${e.toString()}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should use a guard clause instead that way it's clear what code goes with what:

  console.log('Error executing Prettier:', e);

  if (!options.ignoreErrors) {
    const message = `prettier-atom: ${e.toString()}`;
    const detail = e.stack.toString();
    atom.notifications.addError(message, { detail, dismissable: true });
  }
  
  return false;
}

@oriSomething
Copy link
Contributor Author

By the way tell when code is ready so I'll rebase history it before merge

Copy link
Collaborator

@robwise robwise left a comment

Choose a reason for hiding this comment

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

Okay, ready to go! Thanks so much! Let me know when you've squashed and I will merge!

Added checkbox to menu

Ignore errors on save when formatOnSaveSilentError is true
@oriSomething
Copy link
Contributor Author

Done

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.

None yet

2 participants