Skip to content

reconstructing a namespace or dir of namespaces in place #20

Closed
wants to merge 11 commits into from

2 participants

@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.

Alex Baranosky added some commits Dec 24, 2012
Alex Baranosky use *config* to specify users' style preferences, for example `:group…
…-imports-by-package`
63a3a61
Alex Baranosky Revert "use *config* to specify users' style preferences, for example…
… `:group-imports-by-package`"

This reverts commit 63a3a61ecf1b456d862b9a47daaddb96e26362b7.
4b97f33
Alex Baranosky Merge branch 'master' of github.com:technomancy/slamhound 016920f
Alex Baranosky adding ability to `reconstruct-dir` f6f7c46
@technomancy
Owner

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
Owner

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
Owner

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
Owner

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.

Alex Baranosky added some commits Dec 25, 2012
@technomancy
Owner

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
Owner

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
Owner

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... 👎 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
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.