This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2017-06-12
Channels
- # beginners (81)
- # boot (29)
- # cider (7)
- # cljs-dev (147)
- # cljsrn (5)
- # clojure (121)
- # clojure-austin (4)
- # clojure-conj (4)
- # clojure-italy (9)
- # clojure-russia (20)
- # clojure-sg (1)
- # clojure-spec (25)
- # clojure-uk (34)
- # clojurescript (137)
- # cryogen (2)
- # cursive (1)
- # data-science (1)
- # datomic (29)
- # events (9)
- # figwheel (1)
- # hoplon (14)
- # jobs (2)
- # luminus (2)
- # off-topic (7)
- # om (36)
- # onyx (6)
- # parinfer (14)
- # re-frame (13)
- # reagent (74)
- # specter (2)
- # test-check (1)
- # untangled (43)
- # vim (14)
- # yada (36)
@dspiteself no problem.
Seeing as equiv-map is now only used my maps which implement IKVReduce, replacing the every? call with a reduce-kv gives a nice speed up of 3-5x. Would a patch for this be welcome?
We use records as keys in a map. You made the -equiv check cheaper now we just need to reduce collisions https://dev.clojure.org/jira/browse/CLJS-1977
PSA: please remember to run self-host tests when developing patches 🙂
particularly when touching cljs/core.cljc
a common gotcha is needing to prefix some function calls with core/
otherwise self-host tests will fail
@anmonteiro thanks for the pointer. I'll try not to forget!
FYI @dnolen @tmulvaney every call of the private fn equiv-map
is of the pattern (-equiv [coll other] (equiv-map coll other))
where coll has an IKVReduce impl. I don't think the fallback was necessary
could maybe move the reduce-kv impl to PAM and PHM directly and leave equiv-map
untouched
I'm fine with equiv-map being a public helper the implementation isn't hiding anything meaningful
FWIW crossclj doesn't know of any uses, and a search for equiv-map shows some clear copy-pasting.
I agree, but if it is private now, might as well exploit its privateness to tune the impl. if it's gonna be public, it should be cleaned up a little
@dnolen parinfer users can’t contribute patches without creating large diffs. if it helps them and welcomes more newcomers for us, would you consider running a light linter on the repo after merging patches? (changes are small and it runs in ~0.5s)
npm install -g parlinter
parlinter --trim --write "{src,test}/**/*.{clj,cljs,cljc}"
https://github.com/shaunlebron/parlinter@shaunlebron sorry not interested in that.
@dnolen: thanks for looking. let me know if something could’ve made it fit better
I'm also not convinced this will have a significant impact on newcomer contribution. But happy to hear more about why you think it will - lots of people mentioning this problem to you?
thanks for feedback, curious why whitespace is a non-starter (e.g. it respects two-space indentation that I think you prefer)
re: parinfer feedback - I don’t get a lot of it, but Cursive users have complained about this, and i’ve read threads where it is a non-starter not being able to collaborate with non-parinfer users
i designed parinfer to be a newcomer tool, so I think it’s kind of a blow to the whole purpose if they can’t collaborate with it
when, as a parinfer user, I open a file and the editor says 500 lines would have to be changed, I have to refuse and then the file's like a minefield
Just looking at the parlinter diff and there is one change that I'm not fan of, where some code is commented with ;
and the end parentheses are moved to next line. Parlinter moves those parentheses to line before comment.
(ns repl.test
- (:require [clojure.browser.repl :as repl]
+ (:require [clojure.browser.repl :as repl]))
;[clojure.reflect :as reflect]
- ))
@shaunlebron whitespace changes are just noise, why do we want that?
@dnolen: that’s actually not true
I don't see what would the noise matter, perhaps it'd make looking at blame a bit harder
whitespace changes are a one-time diff, future lint usage prevents whitespace changes after that
it’s the same with Prettier/refmt/gofmt in other langs
I agree with Shaun here, I had to re-write some patches just because I accidentally pressed re-indent file in cursive
i find them very aggressive too
The only way this would be considered is it's not invasive for core devs and contributors alike
i deliberately designed this one to be a compromise of current styles while allowing people to opt-in to parinfer
One time style fix is not very invasive? All the problems it fixes seem lime real style problems, bad indentation etc.
there’s a misunderstanding then, the workflow is the same as I understand it, except you run 0.5s command before committing, yeah?
Those problems shouldn't be created when using cursive/vim/emacs/parinfer
Not sure how the current problems have been created
There is style that 90% of the code uses
@shaunlebron but everyone has to do that when they didn't before
@dnolen: true, but you are the gate keeper of patches, and maybe i could write you a script that merges in the patch and lints it
@shaunlebron what about instructing people to solve the problem at git level, locally working on a dirty copy corrected by parlinter and then using some git tools to do interactive staging and create patch from relevant parts only
in a recent experimental project, we run Prettier as a pre-commit hook, so individual developers don't have to manually run the formatter. something like that could be possible. though I definitely understand and appreciate the cautious approach
nobody else would have to care about running it
Pre-commit hooks sounds reasonable if you care about this and there was a style guide formal thing in place
chicken and egg I think, people won’t use it unless it’s collaborative to begin with
What about not touching the dev process but doing one time fix for current "problems"?
I doubt there will be many new problems as people are using Cursive/Emacs etc.
ah I see, it would create a git blame wall
the github interface lets you go back now to previous change though very easily
that was a problem in the past
It is a problem but I'm not sure how big, it touches 6% of the lines which I'd consider quite small change
well, git blame is not that strong argument IMO, you can walk blame history in git/github, it would create one extra hop
@juhoteperi were you looking at just git diff -w
for the 6% number?
git diff --shortdiff
If someone wants to create a branch to show what will happen I will happily take a look
whitespace changes are large, mainly for indented multi-arity function bodies, which clojure-mode and clojure style guide format away anyway
2500 lines, against cloc of 40k
looking
@dnolen: this is the big change:
;; bad
(defn foo
"I have two arities."
([x]
(foo x 1))
([x y]
(+ x y)))
;; fixed
(defn foo
"I have two arities."
([x]
(foo x 1))
([x y]
(+ x y)))
i’ll fork and link the diff
dummy PR for parlinted cljs if we want to track comments: https://github.com/shaunlebron/clojurescript/pull/1/files
@shaunlebron yeah I don’t know it’s just a lot of changes and we have yet a very small amount of evidence that this will effect new contributor contribution
I’d like to get a Paredit/Parinfer question in the Clojure survey to understand impact
but based on last years survey if we assume most people using Atom are using Parinfer that’s quite below 10% of users
@dnolen: I understand. it’s of course hard to measure the effect on potential newcomers but it’s always what I’m looking at—talking to people outside the community about their impressions of lisp, etc. Maybe I can setup a survey outside the community for that
@shaunlebron that would be useful
sure, can I count on you to help share it?
a survey I mean
sure I’ll retweet but I think if you’re looking for a lot of data there’s the obvious places to reach out
thanks, I’ll be targeting the wider JS community though as well
while I may command some “respect” in the JS community I don’t know that anybody actually listens to me 🙂
ha, most will at least hear it, some will listen
@shaunlebron another thing to consider is that I don’t necessarily know that beginners need to start with contributing to the ClojureScript compiler - I mean that’s cool and all but to be honest that’s not something we actually need
it’s always scary to change hashing, pretty sure this is right but people should test it https://github.com/clojure/clojurescript/commit/15779cf5d2e1e55be6d015af92e4d0008eb46bb2
great! thanks so much we will give it a look.
@dnolen what could possibly go wrong? ;) when I screened this for Clojure I did a lot of bytecode reading (to verify the macros were right) and testing of hash values before and after for a variety of cases. Unfortunately, I didn’t keep any of that.
@alexmiller @dnolen I discovered that clojure's type name is hashed strangely
also, hash-coll
, hash-imap
hash-iset
become unused after this, so deleted those too. (They belong to the pre-murmur days)
we had to upgrade a ways and are having issues with relative paths to closure libraries. hopefully I will get you confirmation tommorow
I'm excited about the prospect that we can ratchet up the test quality via generative testing: https://dev.clojure.org/jira/browse/CLJS-2082