OpenParsec / openparsec

GNU General Public License v2.0
51 stars 8 forks source link

Add clear info about codeing style #75

Open tribad opened 2 years ago

tribad commented 2 years ago

Add clear codeing style info into the README.md so that it is visible without searching the repo. Important are tab usage and size And LineEndings not native.

slime73 commented 2 years ago

An .editorconfig file might be handy too - a lot of code editors (including modern versions of Visual Studio) support them. They can have basic stuff like indentation and line ending settings, as well as more advanced and specific code style rules if we want them.

Right now the codebase uses tabs rather than spaces for both indentation and alignment, and CRLF for newlines.

subject721 commented 2 years ago

Having a code style in a README is problematic since it is often ignored due to laziness. An .editorconfig file is great or go one step further and write a .clang-config file. Most editors support it and you can even setup git hooks that enforce the style from the .clang-format file.

uberlinuxguy commented 2 years ago

I don't think we have an "accepted" style at the moment, but he code does have tab stops setup a certain way and line endings that probably should be updated to some form of standard. Perhaps we should zero in on style and such?

slime73 commented 2 years ago

I looked at clang-format a bit, and Visual Studio has some support for it which is cool - but I'm not sure it supports modern versions (including the line ending setting) out of the box. I haven't verified yet but I'd prefer editorconfig if that's the case, because we don't really have complex strict rules that clang-format allows for.

uberlinuxguy commented 2 years ago

One of the windows git clients allows you to choose how to handle line endings when you are installing it. I think, for line endings, we should probably pick one, convert the files, and call it a day. Since Linux and Mac both use the same, (which I think is UNIX style) that's probably the one to support. :-/ I think what is in the repo though is all DOS style line endings... which would mean a bulk find/replace and something that would touch the whole repo.... Not sure if that is a good or bad thing.

Cheeseness commented 2 years ago

Having a code style in a README is problematic since it is often ignored due to laziness.

That doesn't mean it'd be problematic, just that it wouldn't be effective as a standalone solution.

Regardless of whether it's overlooked or not, it's still important to document this sort of stuff somewhere (in addition to any other approaches) so that it can be referred to in cases where a contributor needs guidance or it otherwise needs to be discussed. The README(s) feel like a good place for that.

I think what is in the repo though is all DOS style line endings... which would mean a bulk find/replace and something that would touch the whole repo.... Not sure if that is a good or bad thing.

Couple of thoughts!

Firstly, line ending support isn't an OS thing, it's an application thing, and as I understand it, pretty much every modern text editor on Windows supports \n line endings out of the box (and will automatically use them if a file is opened that already uses them).

Secondly, that line ending config option is a core git thing rather than anything specific to any UIs that sit on top of git. GitHub has some docs about it, including instructions on how to set up behaviours in a .gitattributes file that can be stored in the repo and will automatically be used. This is worth doing IMO, but it obviously it's not a replacement for style guidelines/editor configs for addressing other formatting stuff.

tribad commented 2 years ago

Line Endings are OS specifics. So if your ASCII text files are not conforming to the OS line endings you provoke problems. IDE's are not the only tools that may be used on the code. As I tryed to fix the tab/space chaos I used expand/unexpand to get it all lined up. Unfortunatly unexpand has a problem with CRLF. It does not detect the line end and gives up the conversion. So I needed all the time dos2unix the file, unexpand, do some work and unix2dos the file again. That is anoying. Let git manage the line endings as it is the single source of information for the different build environments.

The use of some file that contains the code-style information is only interesting if you can limit the number of used tools. That is for obviously reasons not the case for open source projects. So as you already found that clang-format/clang-tidy files have no complete support in VS.

So this is why I prefer to have some code-style document and whatever tool some contributor use, he is on his own to make the right configuration for it. This way you will never have problems with changed version and things that do not work in one or other dev environment.

Using clang-format/clang-tidy is good if you run a jenkins job that prevents a pull request from being merged without matching with the rules. If above that some developer can make use of these configs for his own dev environment, that will be fine.

But keep a code-style document for all others.

uberlinuxguy commented 2 years ago

I think the point here may be that .editorconfig can be used for setting indentation settings, as that is mostly "transparent" to the IDE/text editor if it supports .editorconfig stuff. If it doesn't, that's on the dev to figure out how to conform to the code. A doc that states "We prefer you to use X for your tab stops and BLAH for your function defs and code blocks and ... " whatever is useful, even if it's ignored. When someone submits a PR that doesn't conform, a friendly reminder of its existence and a pointer to the .editorconfig is probably a helpful nudge.

The .gitattributes file enforces line endings on the working directory. What we can do is set core.autocrlf in the .gitattributes file to true the the git client will auto convert it based on the OS it's running on (barring the conversation about line endings being OS specific... ;-) ) That change alone should be enough, and not need to enforce working directory line endings on devs. By default, core.autocrlf is set to false in git clients unless the dev changes it. That is probably the source of the current issue.

That will also allow us to find any files that don't conform to LF only and fix them, potentially.... if I understand it right. If I don't, then any CRLF files should be hunted down before we add a .gitattributes file to make sure everything is on level playing field.