[core] Add text operations to manipulate a document by gibarsin · Pull Request #641 · pmd/pmd

gibarsin · 2017-09-21T23:56:38Z

Prologue

Hi to everyone, this is my first contribution to the project! This is the first of the many upcoming changes to the PMD project. This contribution is part of the feature Autofixable issues. Many of the changes which support this feature will be brought by my partner @MatiasComercio and me. Our mentor is @jsotuyod.
Any feedback, sugestion or problem with the following changes are welcome!

Concepts

Document

A document is a representation of a source file which can be read or updated. To represent the order of application of those manipulations, the concept of text operation is introduced.

Text Operation

A text operation can be represented as an insertion, replacement or deletion on a document, at a given offset and of a determined length.
Text operations, in a document, are ordered by their region. When any operation is to be inserted, it must satisfy that its region does not overlap with the region of the existing text operations. This list of text operations correspond to one and only one document, and they may only be applied once.

Region

A region is defined by an non-negative offset and a non-negative length, and represent a region in a document.

Summary

Text manipulations are to be applied to a document after ending the AST traversal where rule violations are found and reported.

jsotuyod · 2017-09-22T01:13:37Z

do not use StringBuffer, it's a synchronized class. You should use StringBuilder instead

jsotuyod · 2017-09-22T01:16:47Z

is it really a good option to have the whole file contents in memory in a single buffer? This lends itself to reallocations as it's being edited, with large chunks of consecutive memory blocks being copied and moved...

This may work as an initial implementation, but I don't expect this to be final

I did it as a proof of concept to test TextOperation, surely it will not be a final implementation. Should I move it to the test package?

jsotuyod · 2017-09-22T01:19:52Z

use stream.toString() instead

jsotuyod · 2017-09-22T01:41:19Z

I'm not sure I understand the reason for the existence of this class

The placeholderOperation defines a region from which all its children operations will be done. As the operations are modeled in a tree manner, if i have a list of children who represent a set of changes, I would not always want to apply a change in the root node which covers all the region after it is applied to my children.

So... this will always be the root text operation? If so...

the name should reflect this better

you shouldn't be able to freely create new instances... maybe have the document create and hold one internally?

instead of freely adding a text operation to another, you could just add operations to the document, meaning you don't need to keep the root of the tree yourself...

Just thinking out loud here

That's right, this will always be the root text operation and we shouldn't be able to freely create new instances, however we currently don't have any class in which to delegate the addition of edits to the tree. I put some thought on adding the operations to the document but that would increase coupling between the Document implementation and the way the text operations are applied, having two reasons to exist, the representation of a source file and the changes it holds.

True, yet there is an inherent coupling between them both, as the root defines a range equals to the document length. That also means, the same tree CAN'T be simply applied to a different document, but is tied to a single one. This can be on the side of the operation, but still, the one-on-one relationship and the binding between doc and operations must be enforced.

Also, the operations is setup as a tree, yet I see no logic to build this tree (just adding nodes to a parent, but not a logic on what nodes should be children of which), so I'm not sure how this would work out, or if a tree structure is even needed.

jsotuyod · 2017-09-22T01:44:41Z

I see no reason why this wouldn't actually happen under normal circumstances...

I'm not sure in what case could I want to overlap regions. If there were any case, I still can't think of a way to put a total order relationship between them, causing that i would not know what operation to apply first.

jsotuyod · 2017-09-22T01:52:38Z

why would we want to do this?

Unnecessary for the time being, I did them because they weren't time consuming, I'll remove it with the rest of those unused methods

jsotuyod · 2017-09-22T01:52:46Z

why would we want to do this?

jsotuyod · 2017-09-22T01:52:54Z

why would we want to do this?

jsotuyod · 2017-09-22T01:53:03Z

why would we want to do this?

jsotuyod · 2017-09-22T01:53:53Z

do we really need this wrapper over a field's method?

It's not needed, it may be an overkill

jsotuyod · 2017-09-23T21:05:49Z

return ~potentialIndex?

According to the Collections.binarySearch method's documentation, the return value is defined as follows:

The index of the search key, if it is contained in the list; otherwise, (-(insertion point) - 1). The insertion point is defined as the point at which the key would be inserted into the list: [...]

I know, that's why I say it... for signed variables under two's complement, you just invert every bit...

Oh my god, I just realised you were using the bitwise operator, I couldn't make any sense of inverting every bit with just a substraction op. Brillant.

- Refactor getIndexFromChild method in TextOperation class - Remove unused methods from TextOperation class - Replace Stringbuilder for Stringbuffer in DocumentImp class - Change PlaceholderTextOperation for RootTextOperation - Add clarifying documentation

- Changed TextRootOperation to TextOperations which hold a list of text operations for a given document and those changes may be only applied once and for that document only. - Reviewed return value of potentialIndex.

jsotuyod self-assigned this Sep 22, 2017

jsotuyod requested changes Sep 22, 2017

View reviewed changes

jsotuyod added an:enhancement An improvement on existing features / rules in:pmd-internals Affects PMD's internals labels Sep 22, 2017

jsotuyod reviewed Sep 23, 2017

View reviewed changes

jsotuyod added the is:WIP For PRs that are not fully ready, or issues that are actively being tackled label Sep 30, 2017

gibarsin force-pushed the text-manipulation branch from 2eb5b1d to 4c87b39 Compare October 6, 2017 17:36

gibarsin added 4 commits October 6, 2017 19:23

Add text operations to manipulate a document

bc9a863

Add fixes from suggestions

2e5fec6

- Refactor getIndexFromChild method in TextOperation class - Remove unused methods from TextOperation class - Replace Stringbuilder for Stringbuffer in DocumentImp class - Change PlaceholderTextOperation for RootTextOperation - Add clarifying documentation

Simplify return value for getIndexToAddChild function

5355416

Update Text operations execution design

c30783d

- Changed TextRootOperation to TextOperations which hold a list of text operations for a given document and those changes may be only applied once and for that document only. - Reviewed return value of potentialIndex.

gibarsin force-pushed the text-manipulation branch from 4c87b39 to c30783d Compare October 6, 2017 22:24

MatiasComercio mentioned this pull request Oct 28, 2017

[core] Autofixes feature development #693

Open

14 tasks

gibarsin closed this Dec 26, 2017

gibarsin deleted the text-manipulation branch December 26, 2017 12:49

adangel added the in:autofixes Affects the autofixes framework label Feb 6, 2018

Uh oh!

Conversation

gibarsin commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prologue

Concepts

Document

Text Operation

Region

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsotuyod Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gibarsin commented Sep 21, 2017 •

edited

Loading

jsotuyod Sep 23, 2017 •

edited

Loading