reconstructing a namespace or dir of namespaces in place

[AlexBaranosky]

Not sure if this kind of feature is something you think fits into the vision you have for Slamhound. It is a way to update the namespace in a file in-place, or update all files' namespaces in a directory structure in-place.

If you think it is not a fit, I'll create some kind of Slamhound extension to house it.

Also, before pulling I want to test it, and probably fix the doc-string etc.

[technomancy]

Yeah, I'd be happy to add this functionality.

The implementation here exposes the wrong things though--there are two operations the user would be interested in: emitting a reconstructed namespace and replacing an existing namespace declaration with a reconstructed one. The latter operation should accept a filename and call file-seq on it, filtering for .clj files, probably as -main. That way you don't have to care about whether you're passing a single file or a directory in. I really would rather avoid naming anything in the style of foo* since it's always possible to find a more descriptive name.

Also, the recursion here isn't necessary; drop would allow for a much more concise implementation.

[AlexBaranosky]

Cool. Didn't know about file-seq. Let me tweak this some more and send it back your way.

[AlexBaranosky]

Were you thinking of something like this?:

(defn- body-from-file [file-name old-ns-form]
  (->> (slurp file-name)
       (drop (count (str old-ns-form)))
       (apply str)))

The reason I did this using loop, was because when you read the ns form then str it, then the length of the chars gets changed (the whitespace gets altered) So I was instead counting non-whitespace as a way to get around that issue.

The above version doesn't work.

[AlexBaranosky]

Ok, so I made a few changes. Let's iterate further on them. Maybe there's a way to avoid the funkiness in body-from-file; that's be great. Also, are the names good? reconstruct-in-place is a pretty bad name actually, I have to admit.

Let me know your thoughts.

[AlexBaranosky]

The counting non-whitespace approach I took is also not perfect. It messes up when the old ns form had comments in it, or has metadata.

[AlexBaranosky]

Normally, I'd test something like this that is all about IO using https://github.com/amitrathore/conjure. But I don't want to add a dependency without talking to you first. Do you have some kind of approach you like to use for testing things like this?

My approach using conjure would be to mock spit, and verify it was called with a namespace string that matches what we expect.

A non-conjure approach would be to expose a version of reconstruct-in-place that doesn't quite write to IO, but instead just returns the string for the entire namespace. We could test that just using (is (= ...)), but it means the source would have to be tweaked to extract that sub-function.

Thoughts?

I'd really like some automated test of this, because I already know there are cases that are broken (comments, ns metadata) and would like to add tests for those cases and try to fix them.

[technomancy]

Of course; I see what you mean. Comments and reader metadata do present a problem.

What about just opening a reader on the file, reading a single form, and then doing io/copy between the reader and the output? It takes advantage of statefulness, but if it's isolated there isn't much chance of that causing problems.

Testing should be easy if we copy a dev-resources file into a tempfile and operate on that. Slower than working in-memory, but more realistic.

[technomancy]

Also, I think reconstruct-in-place is a pretty good name. Maybe the error handling could be moved to -main too?

Thanks!

[AlexBaranosky]

That all makes sense, except one thing.

If I move the error handling into the -main function then any failure will shortcircuit the reconstruction. The way it is now, some namespaces can have issues, but the rest still get reconstructed fine. Since errors here aren't necessarily serious, and may not even be fixable, I like the current behavior.

I'm working on the other ideas still.

[technomancy]

Sure, I can sew the reasoning for wanting it inside the doseq. It's a bit less tidy from an API perspective but the interactive aspect is probably more important here.

[AlexBaranosky]

For some reason, when I put the test .clj files into dev-resources, io/resource wasn't finding them. I'll fix it when I can see what silly thing is going wrong. Other than that, what do you think? Any more feedback?

[AlexBaranosky]

What we could do is make reconstruct-in-place return a seq of result-maps: one for each file we attempt to reconstruct.

Then -main could turn any errors in the result map into println statements.

[AlexBaranosky]

The above commit is an example to show you what I am getting at.

[AlexBaranosky]

Oops. I think that needs to be a for with a doall.

[technomancy]

I realized reconstruct-in-place can be turned into a fairly trivial combination of file-seq, re-find, and swap-in-reconstructed-ns-form, so from an API perspective swap-in-reconstructed-ns-form is more likely to be of interest. The file-seq-using function could just become -main.

[technomancy]

Went ahead and merged this. I think I'm ready to cut 1.3.0 unless there's anything else you think should go in.

[technomancy]

Oh, re: dev-resources are you still on Leiningen 1.x? If so we can add a :dev-resources-path for backwards compatibility.

[AlexBaranosky]

Looks great. We managed to make it really simple. I like that all three functions are now public and meaningful/useful individually. I'm happy with how it is shaping up. Will this be exposed somehow from the lein-slamhound plugin?

Yes, I'm still using Leiningen 1... :-1: I've got a lot of projects form work on Lein 1, so I just never went ahead and changed to lein 2.

Rather than change anything, I'll install a leiningen 2 as well.

[AlexBaranosky]

Ah, I just read your commits. I see, the lein plugin no longer makes sense.

[technomancy]

Yeah, lein1 doesn't have support for partial aliases, but you could approximate it with a shell alias that just called lein1 run -m .... Could suggest this in the docs.