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

Add support for multi-linebreak-preservation #209

Closed
wants to merge 3 commits into from

Conversation

fkirc
Copy link

@fkirc fkirc commented Nov 7, 2020

This is a solution for #207.
I already tested it successfully on the following feature-branch of attranslate: fkirc/attranslate#64
This API-proposal isn't really clean, but I do not dare to make any breaking changes as an external contributor.

@eemeli
Copy link
Owner

eemeli commented Nov 7, 2020

I don't think I like this. On a conceptual level, the AST does not promise to retain the exact shape of the source string. Once it's been passed through once, it'll stay pretty close but even then the docs do include at least this note:

Due to implementation details, the library's comment handling is not completely stable. In particular, when creating, writing, and then reading a YAML file, comments may sometimes be associated with a different node.

So in a sense the library is usable as a YAML prettifier by parsing & stringifying a source string. And as a part of that, it currently cleans up extraneous white space between nodes by collapsing it into a single empty line. This PR muddies the situation by changing the logic to only dropping any trailing white space from those lines, while retaining the complete drop of any empty lines after a comment.

I think I'd like to see much wider support for this change before considering it further. Also, there's no realistic way of making it not a breaking change, and having both spaceBefore and spacesBefore is just not going to happen.

@fkirc
Copy link
Author

fkirc commented Nov 7, 2020

I don't think I like this. On a conceptual level, the AST does not promise to retain the exact shape of the source string. Once it's been passed through once, it'll stay pretty close but even then the docs do include at least this note:

Due to implementation details, the library's comment handling is not completely stable. In particular, when creating, writing, and then reading a YAML file, comments may sometimes be associated with a different node.
So in a sense the library is usable as a YAML prettifier by parsing & stringifying a source string. And as a part of that, it currently cleans up extraneous white space between nodes by collapsing it into a single empty line. This PR muddies the situation by changing the logic to only dropping any trailing white space from those lines, while retaining the complete drop of any empty lines after a comment.

I understand that this PR is not the way things are currently working, therefore I think that we need to step back from this specific PR and instead debate on the general goals of this library.
Preserving the visual presentation of a YAML as much as possible is a valid use case, whereas "prettifying" is another valid use case.
Some users want to have a "prettifier" that aggressively reformats YAML to match some "standard form".
This is not the thing that attranslate wants, attranslate wants the former use case instead (preserve as much structure as possible).
So the crucial question is:

Do you want to support the "preservation use case", or do you only care about the "prettier use case"?

I think I'd like to see much wider support for this change before considering it further. Also, there's no realistic way of making it not a breaking change, and having both spaceBefore and spacesBefore is just not going to happen.

To make it not a breaking change, we would probably need an additional config-option.
I would add such config-option if there is a chance of getting something merged (not necessarily this API-proposal).

@fkirc
Copy link
Author

fkirc commented Nov 13, 2020

Perhaps I should rephrase my question:

Is there anything I could do to improve support for the "preservation use case", without threatening the already existing "prettier use case"?

My point is not to merge this particular PR, but rather to find a direction that leads to an improved format-preservation.

@fkirc
Copy link
Author

fkirc commented Feb 21, 2021

I am closing this PR because it is not a nice fit for the current API.
Nevertheless, I am still aiming to get this feature done in another form.

@fkirc fkirc closed this Feb 21, 2021
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

2 participants