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

Handling broken ns forms #20

Open
marick opened this issue Jun 9, 2013 · 6 comments
Open

Handling broken ns forms #20

marick opened this issue Jun 9, 2013 · 6 comments

Comments

@marick
Copy link

marick commented Jun 9, 2013

One thing you can ask Midje to do from the repl is to load all of the test files. It uses bultitude (either namespaces-in-dir or namespaces-on-classpath) to find files that load. A problem is that new test files can contain errors in the ns statement (especially when they're created by copy-paste-and-edit). Because bultitude silently throws those files away, Midje doesn't load them, so the non-eagle-eyed user can be fooled into thinking all the tests passed, rather than that none of them were run.

(That non-eagle-eyed user would be me: marick/Midje#166 )

I wonder if it would be useful for read-ns-form to allow the reader's complaint here:
https://github.com/Raynes/bultitude/blob/master/src/bultitude/core.clj#L27
https://github.com/Raynes/bultitude/blob/master/src/bultitude/core.clj#L30
... to make it to standard out, standard error, or some state variable that could be queried by client code that cared.

(Probably best would be a second return value done the way Common Lisp does it. But we don't have that upgrade path.)

If one of these seems a reasonable thing, I'll make a pull request.

@Raynes
Copy link
Owner

Raynes commented Jun 9, 2013

Definitely no state.

I suppose it would be reasonable to add an option to print the exceptions to standard error, but it would be better if we could return a vector of [nses errors] or something. The upgrade path isn't a problem, because that's what major version bumps are for, and it could just be an option that makes this happen if it seems like a big deal.

Thoughts?

@marick
Copy link
Author

marick commented Jun 10, 2013

After I posted, I thought an N-argument function might be best

(namespaces-in-dir <dir>) => same result as now
(namespaces-in-dir <dir> :error-log) => [same-result-as-now map-from-namespaces-to-error-info

That is backward-compatible.

It's also similar to Common Lisp multi-value results, so it has pedigree.

@Raynes
Copy link
Owner

Raynes commented Jun 10, 2013

Exactly what I was thinking. I'll accept a pull request adding this!

@marick
Copy link
Author

marick commented Jun 16, 2013

I've been hacking around on this. The core function produces output like this (some keys omitted):

({:file #<File ./project.clj>,
  :status :no-attempt-at-namespace
  :source :standalone
 {:file #<File ./src/bultitude/core.clj>,
  :status :contains-namespace,
  :namespace-symbol bultitude.core
  :source :standalone}
 {:file #<File ./test/bultitude/invalid.clj>,
  :status :invalid-clojure-file
  :source :standalone})

From this, namespaces-in-dir and namespaces-on-classpath (and namespaces-in-jar) are simple matters of filtering and mapping. (Jar files are handled by a lot of the same code, but the :source is :jar-file.)

Does this seem reasonable?

I'll probably put this into a pre-release of Midje before submitting a pull request. A little bashing by Midje-using projects would make me more confident in the changes I've made.

@marick
Copy link
Author

marick commented Jun 16, 2013

Any interest in my converting the tests into Midje? I've been adding tests, and using test-unit is seriously cramping my development style.

@Raynes
Copy link
Owner

Raynes commented Jun 17, 2013

No thanks, I'd rather stick to clojure.test.

The other stuff looks good.

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

No branches or pull requests

2 participants