Current pattern for multiple files formatting

[arichiardi]

Hi zprint community

I was wondering if we are doing the right thing on multiple file path formatting:

zprint -sfw /file/path1 /file/path2 ...

Where the files are white-space separated by a bash scripts (find foo -iregex ...) . We went this route because we cannot just use *.clj in every dir...dirs are many and we have *.cljc, *.edn and *.cljs files in them.

Now, we think something changed with zprint 1.2.8 and with the above pattern but -sfc we noticed many "Requires formatting..." problems (but I don't have yet a repro for that).

My question is - is this the right approach? Is there a better way?

[kkinnear]

Depending on what version you are coming from, you may be seeing things formatting differently because of the "smart wrap" change for comments in 1.2.6. There were also some significant formatting changes in 1.2.5 where I made changes to the tuning parameters to avoid having files that were just formatted end up changing if they were formatted again. In both of these releases the actual "look" isn't wildly different, but there may be plenty of files that get relatively minor changes, even if they didn't have any changes since they were previously formatted with zprint. Certainly if you were upgrading from 1.2.4 to 1.2.8 I would expect you would see changes.

If you were moving from 1.2.7 to 1.2.8, I wouldn't expect anything to have changed much.

In any case, if you can give me a file or two that changed where you wouldn't expect it, and the version you were using, I'll see what I can figure out.

I don't know of anything interesting about the way you are doing files that should be a problem. zprint doesn't know the difference between *.clj and a bunch of files given explicitly, as far as I know. Unless there might be some issue with total size, but that seems unlikely. I keep entire files in a single string with no issues.

[arichiardi]

Yes the funny thing is that I committed the differences and then launched a check against those in CI that failed (but succeeded locally). This was not easily reproducible.

I keep entire files in a single string with no issues.

So are you also passing multiple files as white-space separated to zprint?

[kkinnear]

I'm not entirely sure we are communicating here. When I said "I keep entire files in a single string with no issues", I mean that the source for every file zprint formats spends plenty of time as a single Clojure string. The point of that comment is that there is unlikely to be a size limit on the size of the information coming in from the shell for file specifications that is being hit that wouldn't already be hit by the size of the raw files themselves.

To answer your question directly, not really. I don't typically place multiple filespecs on the same line. But when I say *.cljc, the shell does place multiple filespecs on the line when it expands the *.cljc. So in that sense, anyone who uses a * will usually generate white-space separated filespecs. Won't they? I could be missing something here.

I'm sure you've checked, but is there a possible difference in the zprint config in CI from your local version? Or even a version difference in zprint? Or, (please no), you are doing CI on a different platform than your local work?

[arichiardi]

I'm sure you've checked, but is there a possible difference in the zprint config in CI from your local version? Or even a version difference in zprint? Or, (please no), you are doing CI on a different platform than your local work?

Yeah I sha256sumed it and also checked the output of --explain and everything matched.

To answer your question directly, not really. I don't typically place multiple filespecs on the same line. But when I say *.cljc, the shell does place multiple filespecs on the line when it expands the *.cljc. So in that sense, anyone who uses a * will usually generate white-space separated filespecs. Won't they? I could be missing something here.

If the *.clj expansion we are talking about is done by the shell yes I think that would result in the same whitespace-separated list. Well that answers a question I had actually - I was wondering if *.clj was actually a special zprint thing but it does not seem so.

Now that we are on this I should try and run getconf ARG_MAX on both and see if the value is wildly different.

[kkinnear]

In discussing this with someone, it occurred to me that I have seen situations where there were limitations on the length of a single filespec. This would be not so much in zprint, but in some of the tools that were generating filespecs, perhaps. I'm sure that the lengths of the filespecs produced by find locally are different from those where you run find in your CI system. Almost certainly the CI ones are longer, and I have seen situations where there were limitations where those got trimmed. Not with zprint, but with one of my previous jobs, we had situations come up like that. Probably not what you are dealing with, but something worth a thought perhaps.

[arichiardi]

Yes I think the problem lies there. This brings me to another idea then - would it make sense to add some sort of better support for formatting inside nested dirs? It would remove the need of external scripts at the cost of of course more logic (and code to maintain) in zprint...

[kkinnear]

Hmmm. I'm not sure just what "better support for formatting inside of nested directories" would look like.

I did discover something responding to an issue or discussion question (that I can't actually find right now), which I didn't know. If you do something like this:

%ls **/*.clj

you will see all of the .clj files in the current directory and all of its subdirectories. I have been using Unix-like systems since about 1983 but I never knew about the ** glob character(s). Not all shells support this, I'm sure, but I believe it is widely supported. You probably already know about this.

I'm not sure this would solve the problem -- but then again it just might.

Anyway, I'm not likely to re-implement find inside of zprint, but if you could put some more words around "better support for formatting inside of nested directories", I will at least give it some serious thought.

[arichiardi]

Well yeah I think there are various ways to get a list of files in shells but the problem you will face sooner or later given a sufficiently large repo, is that ARG or filespec limit.

I am sure it is solvable in bash (which we use) but moving the resposibility of finding all the clojure files to format starting from a dir within zprint is very enticing.

Again just food for thought not saying it should be implemented ah ah.

[kkinnear]

Thanks. It wasn't clear to me just what problem you were trying to solve, but I have a bit of a better idea now. I'll see if I can figure out how to do that in a reasonable way -- with a reasonable UI as well.

[kkinnear]

Would something that did essentially this solve your problem? I don't mean actually using the shell, but getting this effect inside of zprint?

zprint -lsfw **/*.clj **/*.cljc **/*.edn

where you get to pick the extensions? That is, start in the current directory, and work down recursively doing all of a specified set of extensions? Where you tell zprint the extensions you want.

That looks pretty easy to implement, though I'd have to switch file system libraries, which is far from easy. There are a lot of quirks in the ways that file systems work that I have fixed up over time using the current file system library that would come undone. But it might be worth doing.

[arichiardi]

It looks great for sure, would ** mean "from current dir"?

That definitely looks good to me and it is nice because I also usually format the bb.edn/deps.edn at the project root.

It reminds me of the gitignore patterns, I like it.

[kkinnear]

I think the ** would mean (like it does for shells), recurse down from here. I think starting in the current directory might be the default, but I suppose that could also be specified. I'll work on finding a way to integrate this into the zprint UI, and get your reaction before I do anything with it.

Presently, I'm looking into reworking the meta output to get you what you want (Issue #312). Making some small architectural changes to see if I can generate exactly what you want instead of the "close" I did before. We'll see how that comes out.

[arichiardi]

Thank you! We finally got to update the formatter and these two plus the namespace sorting are really going to be killer features in terms of time savings and (less) cognitive burden. Do you want me to close the discussion and open an issue instead?

[kkinnear]

No, this has a number that I can use. Don't need an issue. Since it has been a long time since 1.2.8, once I get the meta stuff figured out, I'm likely to do a release and put any file system handling (the stuff we are talking about here), in the next release. Unless it looks really easy, which is possible. I was about to release it before you submitted the meta stuff, but I want to get that handled if I can before releasing 1.2.9.

[kkinnear]

I've been playing around with the code for this. I have to release 1.2.9 before I finish it, but I wanted to see what you think about my approach. My plan so far is to allow something like this: {:files {:directory "." :filespec "*.clj"}} in an options map. But only in an options map that appears on the command line of one of the pre-built binaries. Moreover, if you specified files explicitly to the shell, and you also specified the :files key, that would be an error.

The actual format for the :filespec is quite broad, where things like {:files {:directory "." :filespec "**{.clj,cljc}"}} would be allowed. I'm not wild about the actual form of the :filespec, but messing with it seems like a great way to limit things and introduce bugs, so I'm planning to just go with it as-is. The details are here. Not particularly Clojure-like, but sufficiently complex to not want to mess with. I believe that this will give you sufficient generality/power so that you can probably do all of the files you want with one command invocation.

Opinions?

[arichiardi]

I like the mutually exclusive option as in our case we either format individual files or all of them in the project.

I am not in front of my machine but I recently moved our script to babbasha and I found that the glob function fits perfectly our use cases and it is very easy to use:

https://github.com/babashka/fs/blob/master/API.md#glob

I think you want to have some sort of estabilished format for your filespec...you could even call it :glob as it is something wildly understood.
Lastly, for code you could kind of get inspiration from babashka.fs as that functionality is embedded in the JVM (there is a link in the link above 😃).

Maybe this is more of a 4c here😃

[kkinnear]

Well, yes, babashka/fs is the library I was going to include to get exactly this capability. But now I'm wondering if there is any point in doing so. Are you happy with the babashka script you have now? Is there any point in adding this capability to zprint itself? You are the only folks who have ever asked for this.

Anyway, do you still want me to add this to zprint?

I have the code kind of sketched in enough to see that it can work, but the error conditions are not complete and I've done no tests yet. Actually getting the file names with babashka/fs is trivial -- getting the data out of the options map and handling all of the error cases to give reasonable messages and all of that is the bulk of the work. To say nothing of the tests.

If this would still be useful to you, I'm ok with finishing it. I figure I've done about 1/3 to 1/2 of the work required to complete it.

[arichiardi]

Ok, yes for me/us it has its merits to embed it (less to maintain and not everybody is able to use babasha to wrap zprint). It makes sense to deprioritize it if that we are the only ones that ran into this though. I am still happy with what we have now (compared to the wrapper in bash I had before).

[kkinnear]

I am starting on the next release. If I do this "multiple files formatting" that we have discussed, it will be 1.3.0. I have recovered the work I stashed prior to releasing 1.2.9, and while it needs more development testing as well as long term system tests, I think it is a reasonable thing to do. The additional library also doesn't break my graalVM build, which is always a question.

So I have two questions for you:

1. Would you actually use this capability?
2. I am interested in your thoughts on the UI for this. I was thinking {:files {:directory "." :filespec "**{.clj,cljc}"}}. You suggested replacing :filespec with :glob, which isn't a bad idea. I can't imagine someone using this for a single file. I'm thinking of not requiring a :directory and defaulting to "." if you don't give it.

It would look like this to do what I think you want -- format every .clj or .cljc file in a directory and all of its subdirectories;

% zprintma-1.3.0 '{:files {:glob "**{.clj,cljc}"}}` -lsfw

Does that seem workable to you?

It will be an error to do this (which you might think would do the same thing as the command above):

% zprintma-1.3.0 '{:files {:glob "**.cljc"}}` -lsfw **.clj

You can't have command line files and {:files ...} files in the same command line. You get one or the other.

Anyway, does this still look interesting? I've done what I think is the hard work on it, so it isn't a big deal to finish it off if it would be useful to you (and/or you think it would be useful to others as well).

Thanks!