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

Executable error should be from Executable not Docker installation check #2146

Closed
4 of 5 tasks
stevenzeck opened this issue May 29, 2018 · 2 comments · Fixed by #2148
Closed
4 of 5 tasks

Executable error should be from Executable not Docker installation check #2146

stevenzeck opened this issue May 29, 2018 · 2 comments · Fixed by #2148
Assignees

Comments

@stevenzeck
Copy link
Contributor

Description

Executable errors are not being displayed in the UI. Originated from #2145.

Input Before Beautification

This is what the code looked like before:

N/A

Expected Output

The beautified code should have looked like this:

N/A

Actual Output

The beautified code actually looked like this:

N/A

Steps to Reproduce

  1. Add code to Atom editor
  2. Shut down Docker if you have it installed
  3. Configure a beautifier/executable so it will error intentionally (like setting the wrong path)
  4. Run command Atom Beautify: Beautify Editor

Result: Shows an error "could not find Docker"
Expected: Should show an error about the executable not being able to run instead or in addition to

Debug

Here is a link to the debug.md Gist:

Checklist

I have:

  • Tried uninstalling and reinstalling Atom Beautify to ensure it installed properly
  • Reloaded (or restarted) Atom to ensure it is not a caching issue
  • Searched through existing Atom Beautify Issues at https://github.com/Glavin001/atom-beautify/issues
    so I know this is not a duplicate issue
  • Filled out the Input, Expected, and Actual sections above or have edited/removed them in a way that fully describes the issue.
  • Generated debugging information by executing Atom Beautify: Help Debug Editor command in Atom and added link for debug.md Gist to this issue
@ghost
Copy link

ghost commented May 29, 2018

Please follow the issue template provided. More specifically, update the original comment for this issue by adding a link to the required debug.md gist which includes debugging information that answers our most commonly asked questions. Thank you.

@Glavin001
Copy link
Owner

Glavin001 commented May 30, 2018

The problem is definitely from
https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/executable.coffee#L390-L412

@szeck87 I think this error in Promise.reject(error) is what we want: https://github.com/Glavin001/atom-beautify/blob/master/src/beautifiers/executable.coffee#L396

Would you be able to try out the following change:

  init: () ->
    super()
      .then(() =>
        return @
      )
      .catch((error) =>
        return Promise.reject(error) if not @docker?
        return Promise.resolve(error) # <--- CHANGE HERE
      )
      .then((errorOrThis) => # <----- CHANGE HERE
        shouldTryWithDocker = not @isInstalled and @docker?
        @verbose("Executable shouldTryWithDocker", shouldTryWithDocker, @isInstalled, @docker?)
        if shouldTryWithDocker
          return @initDocker().catch(() => Promise.reject(errorOrThis)) # <---- CHANGE HERE
        return @
      )
      .catch((error) =>
        if not @.required
          @verbose("Not required")
          @
        else
          Promise.reject(error)
      )

Please and thank you 😃 . I think this will fix it!

@Glavin001 Glavin001 assigned stevenzeck and Glavin001 and unassigned Glavin001 May 30, 2018
@Glavin001 Glavin001 changed the title Executable errors not showing in the UI Executable error should be from Executable not Docker installation check May 30, 2018
@helpr helpr bot added the pr-available label May 30, 2018
@helpr helpr bot added pr-merged and removed pr-available labels Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants