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

RFC: Add a 'region' lens to make controlling spacing easier #244

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lutter
Copy link
Member

@lutter lutter commented May 22, 2015

This is not meant to be committed yet, but I would really appreciate feedback based on people's experimenting with these patches.

So far, how deleted text is handled when entries are added to or removed
from the tree is tightly bound up with how the tree was structured,
because the subtree combinator [ .. ] controlled both: construction of new
tree nodes and glomming deleted stuff together.

In addition, the [ .. ] does the glomming based on the key of the tree node
it constructs, so that deleting a node from hte middle of a number of nodes
with the same key had the weird effect that entries tended to 'shift up'.

The region combinator tries to address this, in that all it does is
demarcate how deleted stuff should be glommed, and it does that based on
the position in the input file, not on the value of a key.

Syntactically, this is written as < l >, though other syntax, like 'region
l', or '<< l >>' might be better.

It would be great if this could also be used to indent new things the same
as surrounding stuff, i.e. if we have a lens < l >* and we add en entry in
the middle of it, if that would automatically mean that the new entry gets
the indentation of one of the surrounding lines - that's not implemented
yet.

I would really like to see and hear how this does or does not address some
of the spacing/indentation problems that people have been having, ideally
demonstrated by a simple test.

@raphink
Copy link
Member

raphink commented Jun 12, 2015

I think the feature is really interesting, and would solve a lot of issues (although I'd be more in favor of eventually providing an API exposing the deleted elements so we can control them).

I haven't given it too much thought, but I can't think of a case where it wouldn't be good to make the region behavior the default, instead of patching all the existing lenses.

@lutter
Copy link
Member Author

lutter commented Nov 26, 2015

I just rebased this to latest master.

I am a little hesitant to just change the behavior of [ .. ] wholesale, though it's certainly an option; I wonder though that in lenses where [..] is used on parts of a line we'd get funny spacing behavior, for example in hosts.aug when you delete an alias.

@raphink
Copy link
Member

raphink commented Nov 26, 2015

Do all the tests pass if you change the default behavior?

Also, does it make sense to keep seq entries with region lenses, or could we just get back to static labels, which makes tree manipulation much easier?

So far, how deleted text is handled when entries are added to or removed
from the tree was tightly bound up with how the tree was structured,
because the subtree combinator [ .. ] controlled both: construction of new
tree nodes and glomming deleted stuff together.

In addition, the [ .. ] does the glomming based on the key of the tree node
it constructs, so that deleting a node from hte middle of a number of nodes
with the same key had the weird effect that entries tended to 'shift up'.

The region combinator tries to address this, in that all it does is
demarcate how deleted stuff should be glommed, and it does that based on
the position in the input file, not on the value of a key.

Syntactically, this is written as < l >, though other syntax, like 'region
l', or '<< l >>' might be better.

It would be great if this could also be used to indent new things the same
as surrounding stuff, i.e. if we have a lens < l >* and we add en entry in
the middle of it, if that would automatically mean that the new entry gets
the indentation of one of the surrounding lines - that's not implemented
yet.

I would really like to see and hear how this does or does not address some
of the spacing/indentation problems that people have been having, ideally
demonstrated by a simple test.
@lutter
Copy link
Member Author

lutter commented Mar 8, 2018

Updated to make it possible to switch between the current behavior of [...] and what <...> in this PR did by setting the environment variable AUGEAS_NO_SHIFT. When this variable is set, lenses should not exhibit the 'shifting up' behavior that they usually have any more, but we will lose idempotency to some extent. This update is mostly to make it possible to try this new behavior out inrelation to Issue #450.

It's expected that the tests for this PR fail as-is as I added a test that only works when AUGEAS_NO_SHIFT is set.

@mikhirev
Copy link

mikhirev commented Jul 11, 2018

Hi!

In some rare cases the behavior is incorrect. It can be reproduced with 4 steps: remove a node, save, add a new node, save again. E. g. I created such a file (/tmp/test.ini):

# comment line 1
# comment line 2
# comment line 3
# comment line 4
# comment line 5
# comment line 6

Then I executed augtool built from the dev/region branch and did the following:

% AUGEAS_LENS_LIB=./lenses AUGEAS_NO_SHIFT=1 ./src/augtool --noload --noautoload
augtool> set /augeas/load/Samba/lens Samba.lns
augtool> set /augeas/load/Samba/incl /tmp/test.ini
augtool> load
augtool> match /files/tmp/test.ini/*
/files/tmp/test.ini/#comment[1] = comment line 1
/files/tmp/test.ini/#comment[2] = comment line 2
/files/tmp/test.ini/#comment[3] = comment line 3
/files/tmp/test.ini/#comment[4] = comment line 4
/files/tmp/test.ini/#comment[5] = comment line 5
/files/tmp/test.ini/#comment[6] = comment line 6
augtool> rm "/files/tmp/test.ini/#comment[2]"
rm : /files/tmp/test.ini/#comment[2] 1
augtool> save
Saved 1 file(s)
augtool> match /files/tmp/test.ini/*
/files/tmp/test.ini/#comment[1] = comment line 1
/files/tmp/test.ini/#comment[2] = comment line 3
/files/tmp/test.ini/#comment[3] = comment line 4
/files/tmp/test.ini/#comment[4] = comment line 5
/files/tmp/test.ini/#comment[5] = comment line 6
augtool> ins "#comment" after /files/tmp/test.ini/#comment[3]
augtool> match /files/tmp/test.ini/*
/files/tmp/test.ini/#comment[1] = comment line 1
/files/tmp/test.ini/#comment[2] = comment line 3
/files/tmp/test.ini/#comment[3] = comment line 4
/files/tmp/test.ini/#comment[4] = (none)
/files/tmp/test.ini/#comment[5] = comment line 5
/files/tmp/test.ini/#comment[6] = comment line 6
augtool> set "/files/tmp/test.ini/#comment[4]" "new comment"
augtool> save
Saved 1 file(s)
augtool> quit
%

After that /tmp/test.ini contains

# comment line 1
# comment line 3
# comment line 4
;new comment
# comment line 5
;comment line 6

As you can see, formatting of the last line has changed, although I didn't modify it.

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.

None yet

3 participants