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

Retain line ending by default. #553

Merged
merged 6 commits into from
Jul 28, 2022

Conversation

veelo
Copy link
Contributor

@veelo veelo commented May 29, 2022

If no --end_of_line was specified, find the first line ending in the input and use that for the whole output.

All tests include checking for differences in line endings.

Closes #552.

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems the enum extension should be automatically supported, can you test it also?

@veelo
Copy link
Contributor Author

veelo commented May 31, 2022

What is the enum extension?

@WebFreak001
Copy link
Member

sorry, I meant it looks like --end_of_line=automatic should just work now if we use the std.getopt or std.conv methods with the enum and that it should be documented and tested.

I thought I added a suggestion comment to the review as well but it seems like I didn't.

@veelo
Copy link
Contributor Author

veelo commented Jun 2, 2022

Oh that. The main reason I didn't do that is because then the set of options would be different from EditorConfig options. Before, we had the lf default, now we don't have a documented default just like EditorConfig. When an EditorConfig has no end_of_line defined then it is up to the editor to decide, and our equivalent is that it is up to dfmt to decide, which now takes a hint from the input. So I see the automatic enum member more as an implementation detail than a user visible feature.

Second reason is that is doesn't make much sense to me to supply the default value for an option on the command line.

I'll add a note to the readme what dfmt does when end_of_line is undefined not set.

@veelo veelo force-pushed the retain_end_of_line branch from 9cd071e to 5b995f5 Compare June 2, 2022 07:35
@WebFreak001
Copy link
Member

for IDE tools / integrations it might make sense to know about the special value, so having it documented would be useful there I think.

@veelo
Copy link
Contributor Author

veelo commented Jun 2, 2022

for IDE tools / integrations it might make sense to know about the special value, so having it documented would be useful there I think.

But how will these IDEs handle .editorconfig if it contains a value that is not part of the EditorConfig specification? We shouldn't be forking that specification.

By the way I amended the description:

Property Name Allowed Values Description
end_of_line cr, crlf and lf See EditorConfig documentation. When not set, dfmt adopts the first line ending in the input.

src/dfmt/editorconfig.d Outdated Show resolved Hide resolved
@veelo veelo force-pushed the retain_end_of_line branch from 5b995f5 to a9cb59f Compare June 2, 2022 10:44
If no `--end_of_line` was specified, find the first line ending in the input and use that for the whole output.

All tests include checking for differences in line endings.

Closes dlang-community#552.
@veelo veelo force-pushed the retain_end_of_line branch from a9cb59f to 2ca0bab Compare June 2, 2022 10:45
@veelo
Copy link
Contributor Author

veelo commented Jun 2, 2022

Before now I didn't realize that automatic showed up for dfmt --help. Now it doesn't.

@veelo
Copy link
Contributor Author

veelo commented Jul 10, 2022

This is good to go as far as I am concerned.

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than this minor thing, can you also rebase onto master to trigger the CI with latest code?

src/dfmt/formatter.d Outdated Show resolved Hide resolved
@veelo
Copy link
Contributor Author

veelo commented Jul 12, 2022

I am not seeing any CI. Is that just me?

@veelo veelo force-pushed the retain_end_of_line branch from 0fb2087 to b172b36 Compare July 12, 2022 08:58
@veelo veelo mentioned this pull request Jul 13, 2022
Merged
@WebFreak001 WebFreak001 merged commit 2a4af7a into dlang-community:master Jul 28, 2022
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.

end_of_line defaults to LF, even on Windows.
4 participants