| Message ID | 20230530142317.118873-1-rjarry@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] Add editorconfig file | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Tue, May 30, 2023 at 10:23 AM Robin Jarry <rjarry@redhat.com> wrote: > > EditorConfig is a file format and collection of text editor plugins for > maintaining consistent coding styles between different editors and IDEs. > > Initialize the file following the coding rules in > Documentation/internals/contributing/coding-style.rst > > In order for this file to be taken into account (unless they use an > editor with built-in EditorConfig support), developers will have to > install a plugin. > > Note: The max_line_length property is only supported by a limited number > of EditorConfig plugins. It will be ignored if unsupported. > > Link: https://editorconfig.org/ > Link: https://github.com/editorconfig/editorconfig-emacs > Link: https://github.com/editorconfig/editorconfig-vim > Link: https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length > Signed-off-by: Robin Jarry <rjarry@redhat.com> > --- Great idea, editorconfig is super convenient to have. Acked-by: Mike Pattrick <mkp@redhat.com> > > Notes: > v2: add .editorconfig to EXTRA_DIST > > .editorconfig | 14 ++++++++++++++ > Makefile.am | 1 + > 2 files changed, 15 insertions(+) > create mode 100644 .editorconfig > > diff --git a/.editorconfig b/.editorconfig > new file mode 100644 > index 000000000000..f7f43ecfeea3 > --- /dev/null > +++ b/.editorconfig > @@ -0,0 +1,14 @@ > +# See https://editorconfig.org/ for syntax reference. > + > +root = true > + > +[*] > +end_of_line = lf > +insert_final_newline = true > +trim_trailing_whitespace = true > +charset = utf-8 > +max_line_length = 79 > + > +[*.{c,h}] > +indent_style = space > +indent_size = 4 > diff --git a/Makefile.am b/Makefile.am > index df9c33dfe631..db341504d37f 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -82,6 +82,7 @@ EXTRA_DIST = \ > .ci/osx-build.sh \ > .ci/osx-prepare.sh \ > .cirrus.yml \ > + .editorconfig \ > .github/workflows/build-and-test.yml \ > appveyor.yml \ > boot.sh \ > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 12 Jun 2023, at 15:11, Mike Pattrick wrote: > On Tue, May 30, 2023 at 10:23 AM Robin Jarry <rjarry@redhat.com> wrote: >> >> EditorConfig is a file format and collection of text editor plugins for >> maintaining consistent coding styles between different editors and IDEs. >> >> Initialize the file following the coding rules in >> Documentation/internals/contributing/coding-style.rst >> >> In order for this file to be taken into account (unless they use an >> editor with built-in EditorConfig support), developers will have to >> install a plugin. >> >> Note: The max_line_length property is only supported by a limited number >> of EditorConfig plugins. It will be ignored if unsupported. >> >> Link: https://editorconfig.org/ >> Link: https://github.com/editorconfig/editorconfig-emacs >> Link: https://github.com/editorconfig/editorconfig-vim >> Link: https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length >> Signed-off-by: Robin Jarry <rjarry@redhat.com> >> --- > > Great idea, editorconfig is super convenient to have. I also like the idea, but the editoconfig does not seem to bring much to the table for C. And it looks like there is not much activity on the project. It might be better to look at clang-format, however, this is c/c++ specific. Just my 2c, but we might end up with a configuration file for each person’s favorite editor/linter. > Acked-by: Mike Pattrick <mkp@redhat.com> > >> >> Notes: >> v2: add .editorconfig to EXTRA_DIST >> >> .editorconfig | 14 ++++++++++++++ >> Makefile.am | 1 + >> 2 files changed, 15 insertions(+) >> create mode 100644 .editorconfig >> >> diff --git a/.editorconfig b/.editorconfig >> new file mode 100644 >> index 000000000000..f7f43ecfeea3 >> --- /dev/null >> +++ b/.editorconfig >> @@ -0,0 +1,14 @@ >> +# See https://editorconfig.org/ for syntax reference. >> + >> +root = true >> + >> +[*] >> +end_of_line = lf >> +insert_final_newline = true >> +trim_trailing_whitespace = true >> +charset = utf-8 >> +max_line_length = 79 >> + >> +[*.{c,h}] >> +indent_style = space >> +indent_size = 4 >> diff --git a/Makefile.am b/Makefile.am >> index df9c33dfe631..db341504d37f 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -82,6 +82,7 @@ EXTRA_DIST = \ >> .ci/osx-build.sh \ >> .ci/osx-prepare.sh \ >> .cirrus.yml \ >> + .editorconfig \ >> .github/workflows/build-and-test.yml \ >> appveyor.yml \ >> boot.sh \ >> -- >> 2.40.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Mike, Eelco, Eelco Chaudron, Jun 12, 2023 at 16:54: > I also like the idea, but the editoconfig does not seem to bring much > to the table for C. And it looks like there is not much activity on > the project. It might be better to look at clang-format, however, this > is c/c++ specific. > > Just my 2c, but we might end up with a configuration file for each > person’s favorite editor/linter. The editorconfig project itself does not do much. And since it only affects line length and indent style, I don't expect there should be many improvements. It is less strict that full C formatters like clang-format and uncrustify. But it is simple enough and allows jumping across multiple projects which have all different indent rules and not loose your sanity. In any case, having a .editorconfig file is not mutually exclusive with other tools.
On Mon, Jun 12, 2023 at 10:54 AM Eelco Chaudron <echaudro@redhat.com> wrote: > > > > On 12 Jun 2023, at 15:11, Mike Pattrick wrote: > > > On Tue, May 30, 2023 at 10:23 AM Robin Jarry <rjarry@redhat.com> wrote: > >> > >> EditorConfig is a file format and collection of text editor plugins for > >> maintaining consistent coding styles between different editors and IDEs. > >> > >> Initialize the file following the coding rules in > >> Documentation/internals/contributing/coding-style.rst > >> > >> In order for this file to be taken into account (unless they use an > >> editor with built-in EditorConfig support), developers will have to > >> install a plugin. > >> > >> Note: The max_line_length property is only supported by a limited number > >> of EditorConfig plugins. It will be ignored if unsupported. > >> > >> Link: https://editorconfig.org/ > >> Link: https://github.com/editorconfig/editorconfig-emacs > >> Link: https://github.com/editorconfig/editorconfig-vim > >> Link: https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length > >> Signed-off-by: Robin Jarry <rjarry@redhat.com> > >> --- > > > > Great idea, editorconfig is super convenient to have. > > I also like the idea, but the editoconfig does not seem to bring much to the table for C. And it looks like there is not much activity on the project. It might be better to look at clang-format, however, this is c/c++ specific. It doesn't seem like much, but the automatic reconfiguration of indentation and line width is really handy, especially for newer or infrequent contributors. Especially when the related project DPDK uses almost the exact opposite for settings (and already includes its own editorconfig) > > Just my 2c, but we might end up with a configuration file for each person’s favorite editor/linter. The nice thing about editorconfig is it comes with support for a large number of different editors. But even if we wanted to add a few more similar tools, I don't think that would be a big issue or cause any additional overhead for people who don't want to use those configurations. -M > > > Acked-by: Mike Pattrick <mkp@redhat.com> > > > >> > >> Notes: > >> v2: add .editorconfig to EXTRA_DIST > >> > >> .editorconfig | 14 ++++++++++++++ > >> Makefile.am | 1 + > >> 2 files changed, 15 insertions(+) > >> create mode 100644 .editorconfig > >> > >> diff --git a/.editorconfig b/.editorconfig > >> new file mode 100644 > >> index 000000000000..f7f43ecfeea3 > >> --- /dev/null > >> +++ b/.editorconfig > >> @@ -0,0 +1,14 @@ > >> +# See https://editorconfig.org/ for syntax reference. > >> + > >> +root = true > >> + > >> +[*] > >> +end_of_line = lf > >> +insert_final_newline = true > >> +trim_trailing_whitespace = true > >> +charset = utf-8 > >> +max_line_length = 79 > >> + > >> +[*.{c,h}] > >> +indent_style = space > >> +indent_size = 4 > >> diff --git a/Makefile.am b/Makefile.am > >> index df9c33dfe631..db341504d37f 100644 > >> --- a/Makefile.am > >> +++ b/Makefile.am > >> @@ -82,6 +82,7 @@ EXTRA_DIST = \ > >> .ci/osx-build.sh \ > >> .ci/osx-prepare.sh \ > >> .cirrus.yml \ > >> + .editorconfig \ > >> .github/workflows/build-and-test.yml \ > >> appveyor.yml \ > >> boot.sh \ > >> -- > >> 2.40.1 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 12 Jun 2023, at 17:07, Mike Pattrick wrote: > On Mon, Jun 12, 2023 at 10:54 AM Eelco Chaudron <echaudro@redhat.com> wrote: >> >> >> >> On 12 Jun 2023, at 15:11, Mike Pattrick wrote: >> >>> On Tue, May 30, 2023 at 10:23 AM Robin Jarry <rjarry@redhat.com> wrote: >>>> >>>> EditorConfig is a file format and collection of text editor plugins for >>>> maintaining consistent coding styles between different editors and IDEs. >>>> >>>> Initialize the file following the coding rules in >>>> Documentation/internals/contributing/coding-style.rst >>>> >>>> In order for this file to be taken into account (unless they use an >>>> editor with built-in EditorConfig support), developers will have to >>>> install a plugin. >>>> >>>> Note: The max_line_length property is only supported by a limited number >>>> of EditorConfig plugins. It will be ignored if unsupported. >>>> >>>> Link: https://editorconfig.org/ >>>> Link: https://github.com/editorconfig/editorconfig-emacs >>>> Link: https://github.com/editorconfig/editorconfig-vim >>>> Link: https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length >>>> Signed-off-by: Robin Jarry <rjarry@redhat.com> >>>> --- >>> >>> Great idea, editorconfig is super convenient to have. >> >> I also like the idea, but the editoconfig does not seem to bring much to the table for C. And it looks like there is not much activity on the project. It might be better to look at clang-format, however, this is c/c++ specific. > > It doesn't seem like much, but the automatic reconfiguration of > indentation and line width is really handy, especially for newer or > infrequent contributors. Especially when the related project DPDK uses > almost the exact opposite for settings (and already includes its own > editorconfig) > >> >> Just my 2c, but we might end up with a configuration file for each person’s favorite editor/linter. > > The nice thing about editorconfig is it comes with support for a large > number of different editors. But even if we wanted to add a few more > similar tools, I don't think that would be a big issue or cause any > additional overhead for people who don't want to use those > configurations. I’m fine with adding this, was just wondering if there would be a better option. Acked-by: Eelco Chaudron <echaudro@redhat.com> I guess you could just have your editor figure out indent mode/size based on the file content :) >> >>> Acked-by: Mike Pattrick <mkp@redhat.com> >>> >>>> >>>> Notes: >>>> v2: add .editorconfig to EXTRA_DIST >>>> >>>> .editorconfig | 14 ++++++++++++++ >>>> Makefile.am | 1 + >>>> 2 files changed, 15 insertions(+) >>>> create mode 100644 .editorconfig >>>> >>>> diff --git a/.editorconfig b/.editorconfig >>>> new file mode 100644 >>>> index 000000000000..f7f43ecfeea3 >>>> --- /dev/null >>>> +++ b/.editorconfig >>>> @@ -0,0 +1,14 @@ >>>> +# See https://editorconfig.org/ for syntax reference. >>>> + >>>> +root = true >>>> + >>>> +[*] >>>> +end_of_line = lf >>>> +insert_final_newline = true >>>> +trim_trailing_whitespace = true >>>> +charset = utf-8 >>>> +max_line_length = 79 >>>> + >>>> +[*.{c,h}] >>>> +indent_style = space >>>> +indent_size = 4 >>>> diff --git a/Makefile.am b/Makefile.am >>>> index df9c33dfe631..db341504d37f 100644 >>>> --- a/Makefile.am >>>> +++ b/Makefile.am >>>> @@ -82,6 +82,7 @@ EXTRA_DIST = \ >>>> .ci/osx-build.sh \ >>>> .ci/osx-prepare.sh \ >>>> .cirrus.yml \ >>>> + .editorconfig \ >>>> .github/workflows/build-and-test.yml \ >>>> appveyor.yml \ >>>> boot.sh \ >>>> -- >>>> 2.40.1 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On 6/12/23 17:15, Eelco Chaudron wrote: > > > On 12 Jun 2023, at 17:07, Mike Pattrick wrote: > >> On Mon, Jun 12, 2023 at 10:54 AM Eelco Chaudron <echaudro@redhat.com> wrote: >>> >>> >>> >>> On 12 Jun 2023, at 15:11, Mike Pattrick wrote: >>> >>>> On Tue, May 30, 2023 at 10:23 AM Robin Jarry <rjarry@redhat.com> wrote: >>>>> >>>>> EditorConfig is a file format and collection of text editor plugins for >>>>> maintaining consistent coding styles between different editors and IDEs. >>>>> >>>>> Initialize the file following the coding rules in >>>>> Documentation/internals/contributing/coding-style.rst >>>>> >>>>> In order for this file to be taken into account (unless they use an >>>>> editor with built-in EditorConfig support), developers will have to >>>>> install a plugin. >>>>> >>>>> Note: The max_line_length property is only supported by a limited number >>>>> of EditorConfig plugins. It will be ignored if unsupported. >>>>> >>>>> Link: https://editorconfig.org/ >>>>> Link: https://github.com/editorconfig/editorconfig-emacs >>>>> Link: https://github.com/editorconfig/editorconfig-vim >>>>> Link: https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length >>>>> Signed-off-by: Robin Jarry <rjarry@redhat.com> >>>>> --- >>>> >>>> Great idea, editorconfig is super convenient to have. >>> >>> I also like the idea, but the editoconfig does not seem to bring much to the table for C. And it looks like there is not much activity on the project. It might be better to look at clang-format, however, this is c/c++ specific. >> >> It doesn't seem like much, but the automatic reconfiguration of >> indentation and line width is really handy, especially for newer or >> infrequent contributors. Especially when the related project DPDK uses >> almost the exact opposite for settings (and already includes its own >> editorconfig) I'm a bit concerned that this patch is applying the same config to all the .c|.h files in the repo while not all of them have the same coding style. The most obvious is that linux headers are using tabs, not spaces. Not so obvious - sFlow implementation that was imported as-is from the other source. There are some other examples. I'm afraid that some editors will try to fix existing code while saving unrelated changes. Best regards, Ilya Maximets. >> >>> >>> Just my 2c, but we might end up with a configuration file for each person’s favorite editor/linter. >> >> The nice thing about editorconfig is it comes with support for a large >> number of different editors. But even if we wanted to add a few more >> similar tools, I don't think that would be a big issue or cause any >> additional overhead for people who don't want to use those >> configurations. > > I’m fine with adding this, was just wondering if there would be a better option. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > I guess you could just have your editor figure out indent mode/size based on the file content :) > >>> >>>> Acked-by: Mike Pattrick <mkp@redhat.com> >>>> >>>>> >>>>> Notes: >>>>> v2: add .editorconfig to EXTRA_DIST >>>>> >>>>> .editorconfig | 14 ++++++++++++++ >>>>> Makefile.am | 1 + >>>>> 2 files changed, 15 insertions(+) >>>>> create mode 100644 .editorconfig >>>>> >>>>> diff --git a/.editorconfig b/.editorconfig >>>>> new file mode 100644 >>>>> index 000000000000..f7f43ecfeea3 >>>>> --- /dev/null >>>>> +++ b/.editorconfig >>>>> @@ -0,0 +1,14 @@ >>>>> +# See https://editorconfig.org/ for syntax reference. >>>>> + >>>>> +root = true >>>>> + >>>>> +[*] >>>>> +end_of_line = lf >>>>> +insert_final_newline = true >>>>> +trim_trailing_whitespace = true >>>>> +charset = utf-8 >>>>> +max_line_length = 79 >>>>> + >>>>> +[*.{c,h}] >>>>> +indent_style = space >>>>> +indent_size = 4 >>>>> diff --git a/Makefile.am b/Makefile.am >>>>> index df9c33dfe631..db341504d37f 100644 >>>>> --- a/Makefile.am >>>>> +++ b/Makefile.am >>>>> @@ -82,6 +82,7 @@ EXTRA_DIST = \ >>>>> .ci/osx-build.sh \ >>>>> .ci/osx-prepare.sh \ >>>>> .cirrus.yml \ >>>>> + .editorconfig \ >>>>> .github/workflows/build-and-test.yml \ >>>>> appveyor.yml \ >>>>> boot.sh \ >>>>> -- >>>>> 2.40.1 >>>>> >>>>> _______________________________________________ >>>>> dev mailing list >>>>> dev@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>> >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Ilya, Ilya Maximets, Jun 12, 2023 at 17:47: > I'm a bit concerned that this patch is applying the same config > to all the .c|.h files in the repo while not all of them have > the same coding style. The most obvious is that linux headers > are using tabs, not spaces. Not so obvious - sFlow implementation > that was imported as-is from the other source. There are some > other examples. I was not sure what these rules were so I went with what was written in the docs. Is there a place where these special cases are listed? If not, could you list them here so they can be made explicit? We can either put one .editorconfig file per subtree that has different indenting rules or we can list everything at the root. > I'm afraid that some editors will try to fix existing code while > saving unrelated changes. Editorconfig plugins are not automatic formatters. The only purpose of these is to automatically configure editors to use the project's preferences. Thanks!
On 6/12/23 20:29, Robin Jarry wrote: > Hi Ilya, > > Ilya Maximets, Jun 12, 2023 at 17:47: >> I'm a bit concerned that this patch is applying the same config >> to all the .c|.h files in the repo while not all of them have >> the same coding style. The most obvious is that linux headers >> are using tabs, not spaces. Not so obvious - sFlow implementation >> that was imported as-is from the other source. There are some >> other examples. > > I was not sure what these rules were so I went with what was written in > the docs. Is there a place where these special cases are listed? If not, > could you list them here so they can be made explicit? The build-aux/initial-tab-allowed-files seems to list most of the non-conventionally styled files. > > We can either put one .editorconfig file per subtree that has different > indenting rules or we can list everything at the root. Some files with different styles are in the same subtree, so describing at the root sounds better. > >> I'm afraid that some editors will try to fix existing code while >> saving unrelated changes. > > Editorconfig plugins are not automatic formatters. The only purpose of > these is to automatically configure editors to use the project's > preferences. Well, AFAIU, for example, vim plugin will perform trimming of trailing whitespaces in the whole file on save, if trim_trailing_whitespace is set. > > Thanks! > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Ilya Maximets, Jun 12, 2023 at 21:53: > The build-aux/initial-tab-allowed-files seems to list most of the > non-conventionally styled files. I'll have a look there. Thanks. > Some files with different styles are in the same subtree, so describing > at the root sounds better. Ack. > Well, AFAIU, for example, vim plugin will perform trimming of trailing > whitespaces in the whole file on save, if trim_trailing_whitespace is set. Good point. I'll remove that setting.
On 6/12/23 22:58, Robin Jarry wrote: > Ilya Maximets, Jun 12, 2023 at 21:53: >> The build-aux/initial-tab-allowed-files seems to list most of the >> non-conventionally styled files. > > I'll have a look there. Thanks. > >> Some files with different styles are in the same subtree, so describing >> at the root sounds better. > > Ack. > >> Well, AFAIU, for example, vim plugin will perform trimming of trailing >> whitespaces in the whole file on save, if trim_trailing_whitespace is set. > > Good point. I'll remove that setting. It wasn't really about this particular setting. It was just a counterpoint to the 'plugins are not automatic formatters'. :) In fact, this particular option is most likely safe, because we should not really have any trailing whitespaces in a current code. And this is also not hard to check. It's just an option that I was able to quickly google the implementation for in one particular plugin. I have no idea how any of the other options are implemented as I'm not using these plugins. Best regards, Ilya Maximets.
diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 000000000000..f7f43ecfeea3 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,14 @@ +# See https://editorconfig.org/ for syntax reference. + +root = true + +[*] +end_of_line = lf +insert_final_newline = true +trim_trailing_whitespace = true +charset = utf-8 +max_line_length = 79 + +[*.{c,h}] +indent_style = space +indent_size = 4 diff --git a/Makefile.am b/Makefile.am index df9c33dfe631..db341504d37f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -82,6 +82,7 @@ EXTRA_DIST = \ .ci/osx-build.sh \ .ci/osx-prepare.sh \ .cirrus.yml \ + .editorconfig \ .github/workflows/build-and-test.yml \ appveyor.yml \ boot.sh \
EditorConfig is a file format and collection of text editor plugins for maintaining consistent coding styles between different editors and IDEs. Initialize the file following the coding rules in Documentation/internals/contributing/coding-style.rst In order for this file to be taken into account (unless they use an editor with built-in EditorConfig support), developers will have to install a plugin. Note: The max_line_length property is only supported by a limited number of EditorConfig plugins. It will be ignored if unsupported. Link: https://editorconfig.org/ Link: https://github.com/editorconfig/editorconfig-emacs Link: https://github.com/editorconfig/editorconfig-vim Link: https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#max_line_length Signed-off-by: Robin Jarry <rjarry@redhat.com> --- Notes: v2: add .editorconfig to EXTRA_DIST .editorconfig | 14 ++++++++++++++ Makefile.am | 1 + 2 files changed, 15 insertions(+) create mode 100644 .editorconfig