Make it possible for fish_indent to read files. #3037

Closed
onodera-punpun opened this Issue May 17, 2016 · 12 comments

Projects

None yet

5 participants

@onodera-punpun
Contributor

Right now only something like cat file | fish_indent is possible, however being able to fish_indent ./file would be nice (like how gofmt does it)

@ridiculousfish
Member

Makes sense to me.

@ridiculousfish ridiculousfish added this to the fish-future milestone May 17, 2016
@onodera-punpun
Contributor

While I'm at it, gofmt has the following flag, which would be nice to have as well: -w write result to (source) file instead of stdout

@onodera-punpun
Contributor
function fish_indent --description 'Indenter and prettifier for fish code'
    # This is wrapped in a function so that fish_indent does not need to be found in PATH
    # Also handles some file releated stuff
    if test -f "$argv"
        cat $argv | eval $__fish_bin_dir/fish_indent $argv
    else
        eval $__fish_bin_dir/fish_indent $argv
    end
end

Small modification of fish_indent.fish, probably not the cleanest way of doing this...

@krader1961
Member

Seems reasonable to me as well and should be straightforward to implement without having to know C++ or fish internals. Want to take a stab at it @onodera-punpun?

@krader1961
Member

I wasn't aware that fish_indent had a wrapper script. Is that script even relevant anymore, @ridiculousfish? Since both fish and fish_indent are installed in $__fish_bin_dir, which should be part of $PATH, the script appears to be superfluous.

Too, this sort of thing really belongs in the binary rather than a wrapper script so that developers can make fish_indent then immediately run it as in this example: ./fish_indent -w /tmp/x /path/to/script.fish.

@ridiculousfish
Member

The idea is that fish itself may want to invoke fish_indent, and this shouldn't require that fish live in $PATH to work. However it looks like there's only one builtin invocation of fish_indent (in funced) and so we can just replace that invocation with $__fish_bin_path/fish_indent, and then remove the fish_indent function. That sounds like a good path to me.

As krader says, new options should live in the fish_indent binary, not the wrapper.

@onodera-punpun
Contributor

I'll try editing the C++ file, I have a little bit of go and C knowledge, and it can't be that hard :)

@onodera-punpun
Contributor

onodera-punpun@4a49b8e

So I'm pretty sure it can detect if it should read from stdin or argv now, however I'm not really sure /how/ to read from argv (src = read_file(argv);), can anyone give me some small pointers?

@krader1961
Member
krader1961 commented May 19, 2016 edited

Use the fopen() function to get a file handle and pass that to read_file(). In fact, I see that fish_indent currently ignores any CLI arguments that aren't flags. Which is itself a bug. Replace this line

const wcstring src = read_file(stdin);

with something like the following (off the top of my head so may have bugs):

argc -= optind;
argv += optind;

const wcstring src;
if (argc == 0) {
    src = read_file(stdin);
} else if (argc == 1) {
    FILE *fh = fopen(*argv, "r");
    if (fh) {
        src = read_file(stdin);
    } else {
        // emit a "file could not be opened" error and die
    }
} else {
    // emit a "too many arguments" error and die
}
@onodera-punpun
Contributor
onodera-punpun commented May 19, 2016 edited

Diff: master...onodera-punpun:master

So I think I'm almost done, however I'm struggling writing the actual formatted output to a file:

src/fish_indent.cpp:441:39: error: cannot convert 'const wcstring {aka const std::__cxx11::basic_string<wchar_t>}' to 'const char*' for argument '1' to 'int fputs(const char*, FILE*)'
                 fputs(output_wtext, fh);
                                       ^

https://github.com/onodera-punpun/fish-shell/blob/4867833eede4aa644cf5900059aeea9055f4a15a/src/fish_indent.cpp#L441


Oh, and it would be nice to to make the -w optarg optional if there if actually an input file, and write to that input file by defualt, is something like this possible?

@krader1961
Member

You need to convert it to a narrow C style string: wcs2str(output_wtext).

You could write to the input file by closing it after you've read its contents then open it for writing. Perl has a -i flag to mean "edit in place". I'd recommend using that rather than complicating the processing of the -w flag. You should rename the input file by adding a ".bak" or ".orig" extension. Then open the input file name for writing (do man fopen). That way if anything goes wrong the user still has the original version of the script.

@krader1961
Member

I believe this has been resolved with the changes made by @onodera-punpun.

@krader1961 krader1961 closed this May 28, 2016
@zanchey zanchey modified the milestone: next-2.x, fish-future May 28, 2016
@faho faho modified the milestone: fish 2.4.0, next-2.x Sep 4, 2016
@faho faho added the release-notes label Sep 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment