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

fix(makeArguments): No correct work if editor is emacs. #17

Open
mitsuyoshi4 opened this issue Apr 15, 2019 · 3 comments
Open

fix(makeArguments): No correct work if editor is emacs. #17

mitsuyoshi4 opened this issue Apr 15, 2019 · 3 comments

Comments

@mitsuyoshi4
Copy link

mitsuyoshi4 commented Apr 15, 2019

@lahmatiy I encountered a bug that open-in-editor does't work correctly when editor is emacs.

Background
reference:
aws-amplify/amplify-cli#1246

Bug summary
When aws-amplify/amplify-cli apply an option of add api, an error occurred as below.

Error: Command failed: osascript -e 'tell application "Terminal" to do script "cd /Users/chris/Projects/training/nab2019-aws-workshop/aws-amplify-unicorntrivia-workshop-master; emacs --no-splash \"+1:1\" \"/Users/chris/Projects/training/nab2019-aws-workshop/amplify/backend/api/adminpanel/schema.graphql"'\"
266:268: syntax error: A “"” can’t go after this “"”. (-2740)

I researched this bug and I found out that open-in-editor's escaping logic is wrong.

My proposal

To fix this bug, change lib/open.js as below.

diff --git a/lib/open.js b/lib/open.js
index 4d08053..50df999 100644
--- a/lib/open.js
+++ b/lib/open.js
@@ -33,7 +33,7 @@ function makeArguments(filename, settings) {
     //   {filename}:1:2
     //   => "filename:1:2"
     //
-    .replace(/\{filename\}(\S*)/, function(m, rest) {
+    .replace(/\{filename\}([^\\'"\s]*)/, function(m, rest) {
       return quote(info.filename + rest, settings.escapeQuotes);
     });
 }

Pull request
#16

Please consider that, I hope my pull request will be help to open-in-editor.

Thank you and best regard.

@lahmatiy
Copy link
Owner

@mitsuyoshi4 Thank you! I need a time to validate changes, since it affects all editors not emacs only.

@kaustavghosh06
Copy link

@lahmatiy Would it be possible to merge this PR?

@kaustavghosh06
Copy link

Hey guys, would it be possible to merge this PR sometime soon? This has been pending review for a merge since the last 2 months.

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

No branches or pull requests

3 participants