I'll take a peek my-tomorrow.

Ok, taking my first sips of coffee... Good question on read-string-data ... I'll start by playing in the REPL with your snippets above.

Optimally I think we would skip the whole read-string-data, but for compatibility with the old situation I just re-parsed the string that was coerced into a node

Right. Rebooting my brain. We https://github.com/clj-commons/rewrite-clj/issues/214#issuecomment-1430357842 to TokenNode. But if I understand this is causing issues with: > In clj-kondo I have a predicate string-from-token which just does :lines since normally, the string is inside of :lines but not for coerced string -> node Because a TokenNode is not a StringNode as does not have :lines.

This is the rewrite-clj of olden days that I once vendored: https://github.com/clj-commons/rewrite-clj/tree/f515767e4f07f27389ee2885cb459bd87d903887

and there was no explicit coercion defined here: https://github.com/clj-commons/rewrite-clj/blob/f515767e4f07f27389ee2885cb459bd87d903887/src/rewrite_clj/node/coerce.clj so yes, it would fall back to Object: https://github.com/clj-commons/rewrite-clj/blob/f515767e4f07f27389ee2885cb459bd87d903887/src/rewrite_clj/node/coerce.clj#L46

my "fix" in clj-kondo is to get equality between string node from parse-string and coerce, by re-parsing the pr-str-ed string (which feels clunky)

I could also just do a more broad check for a string inside a general token node

Yeah, I do feel it is awkward.

but maybe on the way through parse-string I might as well rip out the whole read-string-data. why does rewrite-clj do this? to get multiple lines. but why does it need that?

something with edit-multiline: https://github.com/search?q=repo%3Aclj-commons%2Frewrite-clj%20%3Alines&type=code

So your question about why read-string-data I think its intent is to preserve multi-line strings when writing out the nodes (to distinguish \n from an actual newline, for example). But I'd have to test my assumption that it is needed for that.

The fact that :lines is used internally and externally means whatever we do we'd have to preserve that aspect of a string node.

yes

ok, then my PR is what we need to do probably

I'm fine with that

shall I also prepare this for rewrite-clj?

I haven't taken a peek at your PR yet, would you like me to?

it involves moving read-string-data to utils, so we don't get a cyclic dependency and wrapping the string in a string-reader so we can read it with read-string-data

yes, take a peek

Ok, I think your PR is option 2 from our https://github.com/clj-commons/rewrite-clj/issues/214#issuecomment-1430357842? But instead of mimicking the parser, just reuse the appropriate parts of the parser, which is a better way to go.

yes, but using the same implementation as parse-string so you don't use one implementation for the other, and a different one for the other

Ya, much better way to go. Also, good news: I forgot to release our previous fix, so this change to it should not annoy any rewrite-clj users.

lol ok

I can transcribe this to rewrite-clj if you like. Unless you have an itch for commits to rewrite-clj, that's fine too.

I've got to keep my co-maintainer-ship street credit up, maybe? ;)

Sure, go for it!

Also coding is just one small part of it, right. There's the thought, discovery, support, design, etc. You are a great co-maintainer already.

@lee https://github.com/clj-commons/rewrite-clj/pull/231

@lee speaking of duplicate builds and climate change, don't you think this is a bit overkill?

19 in progress, 15 successful, and 68 queued checks

maybe there's a good reason for this high number of builds ;-)

Ha! Yeah, thorough, eh?

Just had a peek at workflows. The Libs Tests seem good (one job per lib we test against means 20 jobs), Native Image Tests seems ok (12 jobs to cover variants), but maybe the granularity of Unit Tests could be coarser (currently 68 jobs). In the general: I'm still kinda looking into de-duping unnecessary job runs on GHA. I want to evaluate this next: https://github.com/marketplace/actions/skip-duplicate-actions. For example, if I've just successfully run job X on source Y in a PR there is no need to rerun this job when merging the PR in source Y is unchanged by the merge.

@lee I'm testing libs in just one 1 job for babashka, it's even in the same runner as the rest of the tests

Rewrite-clj runs the full test suites of current releases of libs that use rewrite-clj. Some of these take a while to run, so parallelization helped here. Some of libs tests have historically been a bit flakey, so being able to rerun a single one after failure is nice.

agreed

everytime I dig into rewrite-clj's implementation I'm like: what a brilliant library it still is

And it was originally just written to update dependencies in lein project files! The original author never imagined it would be used for what is it used for today. Quite cool, yes.

@yannick.scherer

And @rundis for rewrite-cljs work, which was pretty awesome too!

And https://github.com/clj-commons/rewrite-clj/blob/main/README.adoc#contributors over the years!

I'll do a maint pass on rewrite-clj (make sure canaries are up to date) and then cut a release, ya?

sure!

Ok. Back at keyboard. Will attempt publish. First publish since moving from GUI-triggered to git version-tag-triggered, so might be a thing or two to iron out.