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

[Fix #384] Add cljr-auto-sort-project-dependencies #385

Closed
wants to merge 1 commit into from

Conversation

raxod502
Copy link
Contributor

@raxod502 raxod502 commented Jul 18, 2017

  • Add new defcustom cljr-auto-sort-project-dependencies, defaults to nil.
  • Add new function cljr--maybe-sort-project-dependencies.
  • Call new function from cljr-add-project-dependency.
  • Update changelog.
  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • The new code is not generating byte compile warnings (run cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el)
  • All tests are passing (run ./run-tests.sh)
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)
    • No, as existing variable cljr-auto-sort-ns was not documented there

@expez
Copy link
Member

expez commented Jul 18, 2017

Cool. This looks good! I'd like to have a test for this, so I'll hold off a little and see what comes of that cask issue.

@raxod502
Copy link
Contributor Author

raxod502 commented Oct 3, 2017

The Cask issue has disappeared. You can therefore expect some tests to be added here once I get free time.

@expez
Copy link
Member

expez commented Oct 18, 2017

Great 👍

raxod502 added a commit to radian-software/radian that referenced this pull request Oct 22, 2017
Of course, we're still using my fork until [1] and [2] are merged.

[1]: clojure-emacs/clj-refactor.el#392
[2]: clojure-emacs/clj-refactor.el#385
@expez
Copy link
Member

expez commented Nov 17, 2017

Ping @raxod502

@raxod502
Copy link
Contributor Author

Indeed, I'm sorry I still haven't got to this. The only time I can really be sure I will have time is at the end of the semester during winter break. It's on my shortlist.

@raxod502
Copy link
Contributor Author

Sigh. Just when I have some time to work on this, Cask produces more trouble for me: cask/cask#418

Nowadays, I'm experienced enough to work around the problem, so this shouldn't prevent me from adding tests to this pull request.

@raxod502 raxod502 force-pushed the feat/auto-sort-deps branch from 05479fe to f36f405 Compare March 15, 2018 02:32
@raxod502
Copy link
Contributor Author

I added some preliminary tests. For some reason, adding them causes 54 unrelated tests to fail. This boggles my mind. Any idea what's up?

Also, is there any good way to debug the test suite? It seems pretty impenetrable to me, especially given that ecukes --pattern does not seem to actually do any pattern-matching.

@expez
Copy link
Member

expez commented Mar 16, 2018

I don't have any good tips in dealing with ecukes. It's been a huge time sink for me over the years too.

@bbatsov
Copy link
Member

bbatsov commented Mar 16, 2018 via email

@expez
Copy link
Member

expez commented Mar 16, 2018

Why not just kill it?

They still provide value, even if they are a bitch to deal with when something goes wrong, and nobody is interested in re-writing the test suite.

@raxod502
Copy link
Contributor Author

@expez What do you recommend? I can't proceed farther with the tests without the ability to debug things, unfortunately.

@expez
Copy link
Member

expez commented Mar 22, 2018

@raxod502 The first test that's failing is the one for the new feature. It fails with Wrong type argument: stringp, nil. Have you tested that the new feature works manually?

The ecukes tests are pretty fragile, so I'm guessing that if you fix the first failing test all the others will fall in line.

I can't remember what works best for debugging, but you can do side-effecting stuff to debug (e.g. write to a temp file with checkpoints to close in on the offending line) or IIRC you can throw errors and have them show up in the ecukes output.

Sorry about the painful test suite :(

@raxod502
Copy link
Contributor Author

I finally got some time to look at this, only to find that the test suite fails on the latest master from a clean build, too.

See tests.log.

I can't debug this test if other ones are failing.

@raxod502
Copy link
Contributor Author

Ping.

@expez
Copy link
Member

expez commented Aug 14, 2018

Build has been fixed

@bbatsov
Copy link
Member

bbatsov commented Sep 20, 2018

@raxod502 Ping :-)

@raxod502
Copy link
Contributor Author

Thanks for the reminder. I have since started the fall semester at Harvey Mudd College, and likely won't have time to work on this for at least a few months, unfortunately. It remains on my to-do list, however.

@manuel-uberti
Copy link

Any chance for this? :)

@raxod502
Copy link
Contributor Author

Sorry, I've long since stopped using Clojure and clj-refactor.el, and I have a lot of other projects that need attention. I'll close this pull request unless someone else would like to adopt it.

@raxod502 raxod502 closed this Jan 11, 2019
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

Successfully merging this pull request may close these issues.

4 participants