-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Fix recent regressions #68
Conversation
I will provide some examples of why this is a serious problems: First problem: The tests doesn't run properlyThe tests aren't even tested, this can be proved by going into a test and change the indentation to be broken. Then run the test. If you do this in any indentation related test that I've added on master you will see that it passed. And if you apply the change from a30e883 and run the test again you will see that it failed (not only because of the changed file though, but that's the second problem). But to prevent this in the future I've added a testcase with broken indentation that requires it to be indented and then have a check that looks that the buffer is not the same as the file instead of the reverse. This way we can know if it breaks the same way again. Second problem: The tests indent badlyThis can be demonstrated by first fixing the first issue, just by dropping a line and then removing running the tests and look at the following output: Example: let
x = [
(let y = 1; in y)
{ foo = 1; }
[ 1 2 3 ]
x
];
in x becomes let
x = [
(let y = 1; in y)
{ foo = 1; }
[ 1 2 3 ]
x
];
in x and [
1
false
true
https://nixos.org/
{
attr = "set";
}
[
"nested"
"list"
]
"string"
] becomes [
1
false
true
https://nixos.org/
{
attr = "set";
}
[
"nested"
"list"
]
"string"
] And that was fixed in 1f922d7. I haven't reflected at all over if the regex is correct or not. I just grabbed the old one that was in use before the refactoring and cleanups. So it worked before with the one and seems to work now as well with 1f922d7. So at the moment it's to me clearly broken. |
There are some problems with handling empty lists and sets: {
} is being indented as
and {
x = [
];
} as {
x = [
];
} As soon as there is at least 1 element, it seems to work fine. Also this it the reason why it behaves weirdly with |
Another case: {
a = {
x1="abc";
};
b = {
x2 = 1;
};
} is just left as is on attempt to indent. But if you add spaces to |
@matthewbauer can you merge? some melpa users are getting the one with regressions. |
{
a = [
];
} was a problem before this PR. |
Yeah sorry for the delay |
Thank you! Do you know if we can get this update to 18.09 too, or are they doomed until19.03? |
I think we can bump melpa for them! There were some issues with the recent update though that i'd want to make sure are resolved: |
I dropped the wrong branch when I cleaned out stuff in my fork... :-)
But I rebased this on master and it's the same as #67 in all other ways.
This also fixes #64.
It fixes some (for me) quite serious bugs in master.