Skip to content

Let user specify default language context #397

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

Merged
merged 2 commits into from
Nov 17, 2017
Merged

Let user specify default language context #397

merged 2 commits into from
Nov 17, 2017

Conversation

eggsyntax
Copy link
Contributor

@eggsyntax eggsyntax commented Nov 12, 2017

When the user enters an apparent namespace like somelib/, and
the language context is ambiguous, clj-refactor creates a popup
asking the user to specify whether the context is "clj" or "cljs".
This change allows the user to specify a language context which
should be assumed, so that clj-refactor will choose that context
in ambiguous cases rather than creating a popup. Default behavior
is still to create the popup; this change only creates a variable
that lets the user override that behavior.
Tested fairly extensively at REPL; it didn't seem substantial
enough to warrant a new automated test (especially since I've
never worked with Cucumber).

Resolves #394

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
    ** See above
  • 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)
    ** Note: it wasn't clear to me how to add it to the changelog, since the changelog is version-based, and this didn't seem to warrant a version bump. Happy to add it if this is clarified.
  • You've updated the readme (if adding/changing user-visible functionality)
    ** README points to https://github.com/clojure-emacs/clj-refactor.el/wiki#customization, which just says how to pull up the set of available customizations (& this now comes up as part of that set).

Thanks!

When the user enters an apparent namespace like `somelib/`, and
the language context is ambiguous, clj-refactor creates a popup
asking the user to specify whether the context is "clj" or "cljs".
This change allows the user to specify a language context which
should be assumed, so that clj-refactor will choose that context
in ambiguous cases rather than creating a popup. Default behavior
is still to create the popup; this change only creates a variable
that lets the user override that behavior.
Tested fairly extensively at REPL; it didn't seem substantial
enough to warrant a new automated test (especially since I've
never worked with Cucumber).
@expez
Copy link
Member

expez commented Nov 13, 2017

Thanks for digging into this @eggsyntax! 👍

This looks mergeworthy to me but I'll give @benedekfazekas a chance to chime in.

Could you also create a changelog entry? I'm sure other people might be interested in this too! :)

@benedekfazekas
Copy link
Member

benedekfazekas commented Nov 13, 2017 via email

@eggsyntax
Copy link
Contributor Author

@expez "Could you also create a changelog entry?"

Totally happy to! I was going to, but it wasn't clear to me how I should do so, since the changelog is version-based, and I assume this doesn't warrant a version bump. Should I start a new section above the 2.3.1 section?

@expez
Copy link
Member

expez commented Nov 14, 2017

Should I start a new section above the 2.3.1 section?

Yes, something like this

@eggsyntax
Copy link
Contributor Author

Changelog updated. Thanks, y'all!

@expez expez merged commit 408ab1f into clojure-emacs:master Nov 17, 2017
@expez
Copy link
Member

expez commented Nov 17, 2017

Fantastic work, @eggsyntax! Thanks a lot! 👍

@eggsyntax
Copy link
Contributor Author

My pleasure, thanks for accepting it :-)

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.

3 participants