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

Consensus on a style guideline with supported tooling #54

Closed
jerinphilip opened this issue Mar 16, 2021 · 4 comments
Closed

Consensus on a style guideline with supported tooling #54

jerinphilip opened this issue Mar 16, 2021 · 4 comments
Labels
high-priority Needs to be done at the earliest

Comments

@jerinphilip
Copy link
Contributor

There is really no fixed style guideline for bergamot-translator. I'm using vim-codefmt to get something consistent adhering to clang-format. google/styleguide as much as possible. Since the majority of the code adheres to this, I propose to make this standard.

Also no java style setters and getters please, if possible? Can we get rid of the header files with camel-case names as well?

It's getting a bit inconsistent among all of us, might be convenient to get this sorted.

/cc @kpu @abhi-agg

@jerinphilip
Copy link
Contributor Author

jerinphilip commented Mar 25, 2021

Can we get this resolved fast over discussion, at least before the shortlist PR (#69)?

My latest changes seem to be invading places where it shouldn't be - 2450820.

@jerinphilip jerinphilip added the high-priority Needs to be done at the earliest label Mar 27, 2021
@kpu
Copy link
Member

kpu commented Mar 29, 2021

This is C++ not java. For better or worse, we should follow what Marian looks like. snakeCase when Marian does. Use _ at the end of member variable.

@kpu kpu closed this as completed Mar 29, 2021
@abhi-agg
Copy link
Contributor

abhi-agg commented Mar 29, 2021

@kpu @jerinphilip Marian uses this:

  1. Name of the classes/struct like ConfigParser
  2. Class/struct member function names like addOptions()
  3. Name of the files as config_parser.h
  4. Class/struct data member names as lineNo_

So, we can follow this. (Btw, Marian doesn't use snake case naming style if wikipedia is correct).

EDIT: Please say if anyone has any objection to the above proposal. Otherwise, we will go ahead with this style 👍

@kpu
Copy link
Member

kpu commented Mar 29, 2021

Oh you're right 🤕 I had snake case wrong.

Fine point:
If it's a private member variable lineNo_;
If it's a public struct member variable, lineNo;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority Needs to be done at the earliest
Projects
None yet
Development

No branches or pull requests

3 participants