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

Create a coding style guide #41

Closed
jungshadow opened this issue Mar 10, 2015 · 13 comments
Closed

Create a coding style guide #41

jungshadow opened this issue Mar 10, 2015 · 13 comments
Assignees
Milestone

Comments

@jungshadow
Copy link
Collaborator

I'm wondering if we should follow the 1622.2 style guide. I mentioned this in a separate comment to @pstenbjorn a while back. If I were to break them out as a guess:

  • attributes seem to be camel-cased (NB: exception being object_id),
  • elements seem to be title-cased with no separators, and
  • enumerations seem to be lower-cased with hyphens taking the place of spaces.

It would mean a lot of changes, but it would be nice to have an agreed-upon coding convention and a bit more similarity between the two specs, especially as we're planning to consume the ballot models. If everyone agrees, I'll create a pull request with the brief "style guide."

@jungshadow jungshadow added this to the Up for Debate milestone Mar 10, 2015
@pstenbjorn
Copy link
Contributor

@jungshadow for readability I agree that following a consistent style is reasonable and if we are going to seek interoperability with 1622.2 it would be best to have consistent styles.

@cjerdonek
Copy link
Contributor

I would also like to make a plug for using standard line endings (see for example here and here on how to configure git, etc). The files seem to have \r\n line endings while \n plays nicer with Git.

@demcg
Copy link
Contributor

demcg commented Mar 13, 2015

Yes, a style guide makes it easier to maintain code/documents.

@jungshadow jungshadow self-assigned this Mar 17, 2015
@jungshadow
Copy link
Collaborator Author

@cjerdonek Knowing that @pstenbjorn uses a Windows machine to do most of his work and I'm on a Mac, I personally haven't experienced any issues. Are you encountering issues with the files?

@cjerdonek
Copy link
Contributor

Are you encountering issues with the files?

@jungshadow Yes, in that git is flagging any changed lines that don't end in a single newline \n (e.g. when using git-diff or git-log -- see a sample screenshot below). Git has a setting to normalize files to end in a single newline \n (to make things easier for projects having both Windows and Mac OS X committers), but it only works if the files are standardized to end in newlines to begin with.

screen shot 2015-03-16 at 10 13 18 pm

@cjerdonek
Copy link
Contributor

Here is one of the better descriptions of the problem and solution:

core.autocrlf

If you’re programming on Windows and working with people who are not (or vice-versa), you’ll probably run into line-ending issues at some point. This is because Windows uses both a carriage-return character and a linefeed character for newlines in its files, whereas Mac and Linux systems use only the linefeed character. This is a subtle but incredibly annoying fact of cross-platform work; many editors on Windows silently replace existing LF-style line endings with CRLF, or insert both line-ending characters when the user hits the enter key.

Git can handle this by auto-converting CRLF line endings into LF when you add a file to the index, and vice versa when it checks out code onto your filesystem. You can turn on this functionality with the core.autocrlf setting. ...

@cjerdonek
Copy link
Contributor

I checked the line endings of the top-level files in the master branch, and this is what I found:

CRLF: index.html
  LF: README.mdown
CRLF: sample feed for v2.1.xml
CRLF: sample feed for v2.2.xml
CRLF: sample feed for v2.3.xml
CRLF: sample feed for v3.0.xml
CRLF: sample_feed_for_v4.0.xml
  LF: sample_feed_for_v5.0.xml
CRLF: spec_v2.2_documentation.html
  LF: vip_spec.xsd
  LF: vip_spec_pending.xsd
CRLF: vip_spec_v2.1.xsd
CRLF: vip_spec_v2.2.xsd
CRLF: vip_spec_v2.3.xsd
CRLF: vip_spec_v2.3_documentation.html
CRLF: vip_spec_v3.0.xsd
CRLF: vip_spec_v3.0_documentation.html
CRLF: vip_spec_v4.0.xsd
CRLF: vip_spec_v4.1.xsd
CRLF: vip_spec_v5.0.xsd

Also, sample_feed_for_v5.0.xml seems to be encoded in UTF-16, as opposed to UTF-8 like all the others.

@jungshadow jungshadow changed the title Should there be a coding style guide? Create a coding style guide Mar 17, 2015
@jungshadow jungshadow modified the milestones: Version 5.0, Up for Debate Mar 17, 2015
@jungshadow
Copy link
Collaborator Author

@cjerdonek Sounds doable. I'll add a .gitattributes file to the vip5 branch and add a separate issue for that particular update. That said, since we'll have a .gitattributes file that enforces LF endings, I don't think it's worth mentioning it in the coding style guide.

@cjerdonek
Copy link
Contributor

@jungshadow awesome, thanks!

@cjerdonek
Copy link
Contributor

A few other conventions that I noticed would be useful for a style guide:

  • all files encoded in utf-8
  • spaces instead of tabs (or vice versa)
  • no trailing white space at the end of lines

@demcg
Copy link
Contributor

demcg commented Apr 2, 2015

Can we add a section on attribute ordering in XSD? something like:

  1. name
  2. type
  3. minOccurs
  4. maxOccurs

If this is deemed a good idea, I can refactor the current XSD when most of the pull requests have been merged. This will avoid merge conflicts.

demcg pushed a commit that referenced this issue Apr 2, 2015
demcg pushed a commit that referenced this issue Apr 2, 2015
@demcg
Copy link
Contributor

demcg commented Apr 2, 2015

I collected the suggestions from this issue into StyleGuide.mdown on the branch feature/StyleGuide.

@cjerdonek - The spaces vs tabs issue is still open, however to get going, I suggested 4 spaces to the tab.

@cjerdonek
Copy link
Contributor

@demcg Great!

Re: tabs vs spaces, yes, I think four spaces is more common. FWIW, all projects I have been involved with have preferred four spaces. I believe that is also git's default.

Re: attributes, I would clarify that you mean attribute names as opposed to attribute values. Also, I think "snake_case" across the board makes more sense for attribute names (e.g. office_id as opposed to officeID or officeId), as opposed to having different naming conventions for different types of attributes (e.g. with or without "ID"). This also parallels Python's naming conventions, where class names are CamelCase with a leading capital, and method, function, and attribute names are snake case. Either way, I think it would be an important feature that all attribute names follow the same rule.

Can you make a PR for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants