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

Changes in example ns according to issue #64 #65

Merged
merged 1 commit into from
Aug 24, 2013

Conversation

Gonzih
Copy link
Contributor

@Gonzih Gonzih commented Aug 18, 2013

According to conversation in issue #64 there is better to use [] in ns declaration after
special forms (:require, :use) since it's clearly describes data. I think same can be
applied to :import and any other form in ns macro.

@bbatsov
Copy link
Owner

bbatsov commented Aug 18, 2013

I'm OK with the change, but let's see what others think as well.

@michaelklishin
Copy link
Contributor

I haven't seen the short form used anywhere and [clojure [string :as string]] is pretty objectively confusing. -1 to mentioning the short form at all.

@Gonzih
Copy link
Contributor Author

Gonzih commented Aug 18, 2013

https://github.com/ptaoussanis/carmine/blob/master/src/taoensso/carmine/connections.clj

Here is short form in action. People use it not very often, but I think we still should mention it.

@Gonzih
Copy link
Contributor Author

Gonzih commented Aug 18, 2013

@michaelklishin I updated my commit, fixed weird situation with string :as string.

@ToBeReplaced
Copy link

If we go this direction, I agree with @michaelklishin and would recommend only the long form. This has the added benefits of working in Clojurescript (the short form does not IIRC) and has the least overhead to understand.

@Gonzih
Copy link
Contributor Author

Gonzih commented Aug 18, 2013

I didn't thought about ClojureScript version before. Good point, thanks,
added additional commit.

On Sun, 2013-08-18 at 08:40 -0700, ToBeReplaced wrote:

If we go this direction, I agree with @michealklishin and would
recommend only the long form. This has the added benefits of working
in Clojurescript (the short form does not IIRC) and has the least
overhead to understand.


Reply to this email directly or view it on GitHub.

Regards,
Max

@Gonzih
Copy link
Contributor Author

Gonzih commented Aug 24, 2013

So what now? Should it be merged/rejected? Any changes required?

@michaelklishin
Copy link
Contributor

LGTM now.

@bbatsov
Copy link
Owner

bbatsov commented Aug 24, 2013

Yep, LGTM too. Just squash the two commits into one.

According to conversation in issue bbatsov#64 there is better to use [] in ns declaration after
special forms (:require, :use) since it's clearly describes data. I think same can be
applied to :import and any other form in ns macro.
@Gonzih
Copy link
Contributor Author

Gonzih commented Aug 24, 2013

Done.

On Sat, 2013-08-24 at 11:56 -0700, Bozhidar Batsov wrote:

Yep, LGTM too. Just squash the two commits into one.


Reply to this email directly or view it on GitHub.

Regards,
Max

bbatsov added a commit that referenced this pull request Aug 24, 2013
Changes in example ns according to issue #64
@bbatsov bbatsov merged commit c690ebe into bbatsov:master Aug 24, 2013
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