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

added editorconfig #2403

Merged
merged 3 commits into from
Sep 16, 2019
Merged

added editorconfig #2403

merged 3 commits into from
Sep 16, 2019

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Sep 12, 2019

Closed #2401.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

@StrikerRUS looks good to me! I just had one recommendation about handling indentation in Makefiles.

indent_size = 2

[*.{py,sh,js}]
indent_size = 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

😆 one day @Laurae2 and I will agree on indents for .R. But today is not that day.

charset=utf-8
trim_trailing_whitespace = true
insert_final_newline = true
indent_style = space
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm new to editorconfig so just reading the docs for the first time, but I think we need this:

# tabs matter for makefiles
[Makefile]
indent_style = tab

Tabs are necessary in Makefiles (link) and I think this config as written would change them to spaces.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have two makefiles in the R package currently (part of the way you use R libraries that link to compiled code)

actually, I think it needs to be

# tabs matter for makefiles
[*{Makefile,Makevars,Makevars.win}]
indent_style = tab

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I guess they are currently empty...well better to be safe. There is also https://github.com/microsoft/LightGBM/blob/master/docs/Makefile and (now that I'm looking in the sphinx stuff), it should probably be:

# tabs matter for makefiles
[*{Makefile,make.bat,Makevars,Makevars.win}]
indent_style = tab

@StrikerRUS
Copy link
Collaborator Author

@jameslamb

I'm new to editorconfig so just reading the docs for the first time ...

Yeah, me too! 😃

Thanks a lot for your very important remark about Makefile! I tried to make the most complete mix from results of the first 10 pages https://github.com/search?q=filename%3A.editorconfig+makefile&type=Code.

insert_final_newline = none

# Tabs matter for Makefile and .gitmodules
[{makefile*,Makefile*,*.mk,*.mak,*.makefile,*.Makefile,GNUmakefile,BSDmakefile,make.bat,Makevars*,*.gitmodules}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

so comprehensive! Amazing, thank you

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

LGTM

@StrikerRUS
Copy link
Collaborator Author

@guolinke Don't you know any other files that need special treatment?

@guolinke
Copy link
Collaborator

@StrikerRUS I think no

@StrikerRUS StrikerRUS merged commit 0237492 into master Sep 16, 2019
@StrikerRUS StrikerRUS deleted the editorconfig branch September 16, 2019 11:38
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup editorconfig
3 participants