atomist-attic / rug

DEPRECATED Runtime for Rugs
GNU General Public License v3.0
53 stars 13 forks source link

Prune the TreeNodes #618

Closed jessitron closed 7 years ago

jessitron commented 7 years ago

Goodbye, MutableContainerTreeNode!

This PR removes a lot of code that was only used by itself, or in places that are better off using different implementations of PositionedTreeNodes.

I condensed the commits only meaningfully for now. It's fine to squash the whole thing on merge if that's what we want.

Meaningful changes include: YamlFileType has its own PositionedTreeNode impl now, which is mutable, because that's the style it uses to build it. Nobody else needs mutable ones.

Antlr parsing also gets its own impl of PositionedTreeNode, because its naming strategy actually needs the value of some nodes. PositionedTreeNodes generally do not retain their values, only their positions. (conceptually, that is. They have a value method because they implement TreeNode, and changing that is for another day.)

TypeUnderFile now declares that it supplies OverwritableTextTreeNodes, because it does.

ddgenome commented 7 years ago

Squash?

jessitron commented 7 years ago

It's fine to squash on merge. I think the PR review is easier with separate commits

kipz commented 7 years ago

I think in general we need to squash before creating PR because the reviewer can't be sure when we should squash and when we shouldn't. It adds a lot of overhead to the reviewer to figure this out.

Also - as a team, didn't we agree this already?

ddgenome commented 7 years ago

I compiled and ran tests in the CLI and runner compiled against this branch, all tests passed.

jessitron commented 7 years ago

@kipz no I don't agree with that yet. and I said in the description to please squash. Would it help if I put that in all caps and right at the beginning?

jessitron commented 7 years ago

@dd oh wow, thank you for doing that. It's a good idea, and I meant to do it and then forgot.

I don't think it needs a changelog entry because the interface should be unaffected, no one is supposed to notice... but maybe it's still worth it? what's the protocol for that?

jessitron commented 7 years ago

@johnsonr Please check the first two commits in particular, and also this one: https://github.com/atomist/rug/pull/618/commits/7f35d26f6334a1e6d8111f22c815c41a9f5f53d0

Those shouldn't produce observable functionality changes but you might know something I don't.

Then squash-merge if you're good with it

johnsonr commented 7 years ago

I find multiple commits can be useful sometimes. I don't mind reviewing and squash merging. It can add information about how the committer to the end result. However, this one does have a lot of commits :-)

johnsonr commented 7 years ago

I applaud the simplification. I had one question I've added a review comment. If there's a good answer to that, I'm happy for you to squash merge.

jessitron commented 7 years ago

There's an answer to that but it's not easy. Let's talk live tonight

jessitron commented 7 years ago

We made the test valid again, and it passed. Once this is green it's time to squash-merge