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

support nginx-mode with crossplane #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

baysao
Copy link

@baysao baysao commented Jun 30, 2019

No description provided.

@lassik
Copy link
Owner

lassik commented Jul 1, 2019

Thank you. This doesn't currently work reliably because crossplane doesn't support stdin properly. I'll send a pull request to crossplane to fix it.

@lassik
Copy link
Owner

lassik commented Jul 10, 2019

It's quite complex because the crossplane formatter uses a parser that visits include files. This means that even if the formatted file comes from stdin, it needs to have a fake filename used to resolve the includes. And those included files need to exist. I don't have time to look more deeply into it right now, but I can try again later.

@lassik
Copy link
Owner

lassik commented Jul 10, 2019

Basically, for formatting purposes, the nginx conf file should always be read from stdin. (Even if we're formatting an existing file, because we may not have saved the file in Emacs, so the latest buffer contents need to be sent from the Emacs buffer to crossplane's stdin. It's simpler and more reliable to always do it via stdin, even if we have saved the file in Emacs just before formatting.)

Then there's the question of resolving includes. I hope we can change crossplane so it doesn't visit includes when formatting a file. If we can't do that, then Emacs needs to give crossplane a fake filename for stdin (some formatters have this command line option, e.g. prettier --stdin-filepath). Crossplane would then use this filename (well, its directory part) to resolve includes. The trouble is, it's possible to create a temp buffer in Emacs with no filename, put that buffer into nginx-mode, and then call format-all-buffer. Then Emacs can't even generate any fake filename for crossplane to use. We could look at the default-directory of the temp buffer, but that's very unreliable because people don't usually intend to create a temp buffer in any particular directory, so they are usually not aware that the default-directory is being used for anything.

@baysao
Copy link
Author

baysao commented Jul 18, 2019

My current temporary solution is wrapper via shell script (not works in Windows) to support stdin as input.
file script: /usr/local/bin/nginx_format.sh

#!/bin/sh
tmp=$(mktemp)
cat - > $tmp
crossplane format $tmp
rm -f $tmp

and define formatter for nginx-mode as

(define-format-all-formatter crossplane
  (:executable "/usr/local/bin/nginx_format.sh")
  (:install (macos "pip install crossplane"))
  (:modes nginx-mode)
  (:format (format-all--buffer-easy executable)))

@lassik
Copy link
Owner

lassik commented Jul 18, 2019

It's great that you got it to work for yourself with a tempfile!

Since it's open source, let's send them a pull request to fix it at the source though. That will make life easier for every editor plugin that uses it to format nginx conf files. Do you know Python?

The code for the crossplane format subcommand is here: is here: https://github.com/nginxinc/crossplane/blob/master/crossplane/formatter.py So it's extremely simple. It calls parse() to do all the difficult stuff.

parse() is defined here: https://github.com/nginxinc/crossplane/blob/master/crossplane/parser.py So it keeps a list, includes, of files to process. First the list has only one file. That's the file from the command line. It parses that file using _parse(). But if the conf file include other files, it adds those files to the includes list and parses them also:

for fname in fnames:
    # the included set keeps files from being parsed twice
    # TODO: handle files included from multiple contexts
    if fname not in included:
        included[fname] = len(includes)
        includes.append((fname, ctx))

We should probably make that part optional. So that it does includes.append((fname, ctx)) only when the subcommand is not crossplane format. This could probably be done by adding a boolean argument to parse() to disable include processing.

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