Codestyle and formatting rules #34
Conversation
| > I noticed that both the FanChart view and the Ahnentafal view had an "Ahnentafel" class. I was concerned about there being more than one window.Ahnentafel. Both views seemed to work, but I was seeing some odd problems during integration into WikiTree.com and thought it wouldn't hurt to change this. I modified the class name in the Ahnentafel view (to be "AhnentafelAncestorList"). | ||
|
|
||
| #### Solutions | ||
| 1. Agree on some naming conventintions that would prevent this, e.g. use of prefix |
There was a problem hiding this comment.
This needs discussion and the agreement on how to solve this...
|
I like your suggestions Michal.
Sorry, Brian, for barging in on the Ahnentafel name for my class ... I
think I started it before I realized you were including your view ...
though, by the time I did that Pull Request, it was already there if I'd
only paid attention to it! Your solution seems to have fixed the problem
though, if one had occurred.
Michal - the one area I violate all the time is the use of quotes - I seem
to use double and single quotes interchangeably - based on whether my
pinky hits the Shift key fast enough or not (and my editor auto-completes
whatever one I use at the time). I'm hoping that the Prettier plugin in VS
Code will fix that for me - and - now I'm more aware of that as an
"offense" to our style guide.
The one thing that might be a bit of a pain is to rename our files - I
think most of us have used camel or Pascal case for the actual file names,
not lower snake case. IF it's a group consensus to change to this, I will
comply. I'm hoping that GitHub is robust enough that a bunch of file
renaming won't make it throw a fit!
- Greg
…On Tue, Oct 11, 2022 at 3:14 PM Michal Vašut ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In docs/codestyle.md
<#34 (comment)>
:
> +| Files | File names must be all lowercase and may include underscores (_) or dashes (-), but no additional punctuation. Follow the convention that your project uses. [[source](https://google.github.io/styleguide/jsguide.html#file-name)] - most Unix-based servers are case sensitives, but this doesn't apply for windows servers.|
+
+### Legend
+
+* `camel case` - a way to separate the words in a phrase by making the first letter of each word capitalized and not using spaces, e.g. `familyName`, `LastName`, ...
+* `upper snake case` - words are written in uppercase and separated with underscores, e.g. `DEFAULT_COUNT_OF_GENERATIONS`
+* `Pascal case` - same as `camel case`, but the first letter is capitalized, e.g. `WikiTreePerson`
+
+### Conflicting names
+
+#### Problem
+
+> I noticed that both the FanChart view and the Ahnentafal view had an "Ahnentafel" class. I was concerned about there being more than one window.Ahnentafel. Both views seemed to work, but I was seeing some odd problems during integration into WikiTree.com and thought it wouldn't hurt to change this. I modified the class name in the Ahnentafel view (to be "AhnentafelAncestorList").
+
+#### Solutions
+1. Agree on some naming conventintions that would prevent this, e.g. use of prefix
This needs discussion and the agreement on how to solve this...
—
Reply to this email directly, view it on GitHub
<#34 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3JDGBTNY3P64J2AQSFRZFLWCW37XANCNFSM6AAAAAARCSOKGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Hello Greg, first of all, this is imho the recommendation rather than enforcement. If you are using VSCode, there is useful setting, type
The thing with filenames depends on consensus - I'll mark the pull request as DRAFT, because it needs a few things to be solved. |
|
My initial thoughts about filenames mirror Brian's .. I like them to
mirror the contents, especially if it is a self-contained class.
It's been a while since I've had to wrestle with competing operating
systems .. wouldn't it be nice if they just played nice?
I can adapt, though, if there is a need for it.
-Greg
:-)
…On Tue, Oct 11, 2022 at 4:31 PM Brian Casey ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In docs/codestyle.md
<#34 (comment)>
:
> +
+| Formatter | configuration file | Homepage | Editor integration |
+| --- | --- | --- | --- |
+| Prettier | [`.prettierrc`](../.prettierrc) | [https://prettier.io/](https://prettier.io/) | [VSCode](https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode), [WebStorm](https://plugins.jetbrains.com/plugin/10456-prettier), [Sublime Text](https://packagecontrol.io/packages/JsPrettier) and lot of others... check the [official website](https://prettier.io/docs/en/editors.html)|
+
+## Naming conventions
+
+| Object | Convention |
+| --- | --- |
+| Variables | `camel case` with lowercase first letter, e.g. `familyName` |
+| Booleans | We should use `is` or `has` as prefixes, e.g. `isDead`, `hasWife` |
+| Functions | Same as with variables + should use descriptive nouns and verbs as prefixes, e.g. `getName()`, `generateReport` |
+| Constants | Should be written in `upper snake case`, e.g. `DEFAULT_COUNT_OF_GENERATIONS` |
+| Classes | Should be written in `Pascal case`, e.g. `WikiTreePerson` |
+| Methods | Same as functions |
+| Files | File names must be all lowercase and may include underscores (_) or dashes (-), but no additional punctuation. Follow the convention that your project uses. [[source](https://google.github.io/styleguide/jsguide.html#file-name)] - most Unix-based servers are case sensitives, but this doesn't apply for windows servers.|
I'm torn on the filenames. I get the Google style guide suggestion. But I
sort of like filenames that match their contents. For example, if there's a
JS file that just contains a particular class, I like naming it
ClassName.js, matching the "obj = new ClassName" formatting of the class
itself. And from my own habits, I find if I use snake_case to make a long
filename readable, I end up using that for variables sometimes too, rather
than being consistently camelCase.
But obviously filenames will end up with a mixture of formats if we use
both PascalCase and camelCase for the names, depending on whether it's a
Class file or not. And I suppose all-lower-case filenames may be helpful
for people moving from one OS to another (I've run into issues in OSX with
TLA due to filename case expectations).
I'm curious if anyone else has any opinions on the matter.
—
Reply to this email directly, view it on GitHub
<#34 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3JDGBQK6PKXP4YP5ITZF6DWCXFDJANCNFSM6AAAAAARCSOKGQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
| | Rule | Value | Note | | ||
| | --- | :---: | --- | | ||
| | Line length | 120 characters | | | ||
| | Indentation | 4 spaces | | |
There was a problem hiding this comment.
This is my personal preference - I'm python coder and this is golden standard in python. I've thought about 2 spaces (also suggested by Google JS guidelines) and it's more compact, but I'm used to 4 spaces and those 2 doesn't look so clear and it's IMHO harder to orient, but thats what I feel.
I'm against using tabs
- editors can be set to make spaces on tab key press
- Github makes ver large spaces for tab
- it doesn't really matter it you save some characters (1 character for tab vs N characters for every space)
There was a problem hiding this comment.
I agree. Two spaces is not enough, and we should definitely use four. I also agree they should be actual spaces. Editors can just do the replacement so the tab key itself works as expected. But storing tab characters in the file means everything gets out of alignment when viewed later.
The .prettierrc file that's currently in the repo sets tabWidth to two (2) spaces for HTML and CSS files. Is there a reason not to just have those files also use four-space indentation? (It also has an "override" for .js files, but that sets the tabWidth to the default 4; that's probably unnecessary.)
|
Oh, I missed the discussion. Yes, I agree that the file name should match the contents. And mine did, I changed them, and plan to change them back since I see this discussion. git will just have to deal with it. On some long ago project I had to learn to convert from 4 space to 2 space indentation. The only time it becomes an issue is with a deep hierarchy. That is something that might be seen in an html file or a Vue . Some of the existing code does have tabs in it, on the browser. I noticed it today editing with vim. (Yes, old school but if you can type its faster) |
|
My personal preference would be to stick with four spaces everywhere. Deep hierarchies can get wide, for sure, but I feel it's unusual for it to be a problem and would prefer the most common state to be more readable. But I'm happy to concede a 2-space indent for HTML, if that's what others prefer. I don't see it being a problem ever for CSS, though, which is currently one of the 2-space exceptions in the .prettierrc config. Vim is great, and pretty much always available. I still frequently use it myself, though I confess I've grown to like some of the features in VS Code. I do miss being able to pipe sections out to awk or something for big scripted edits. Every IDE has a tradeoff, I suppose. I'm not inclined to make a big/separate project of reformatting existing code to match the draft/developing code style. My thought would be to just clean that up as other edits are done to particular files/sections, and try to follow the guidelines for new code. |
|
My personal vote would be 2 spaces for indentation. It is the default in prettier and what the Google standards suggest. It is also what I use in all my projects. Personally I would suggest doing a one off reformat of everything. Otherwise, if people are using format on save then every PR for a while is going to be a mixture of reformatting and actual significant changes. Which makes it hard to review. |
|
What about this one? There are still ongoing discussions mainly about two things
And every vatiation has its pros & cons (supporters & "haters"). We can let the discussion going further or maybe it would be better to let some authority ( let's say owners of the repo) decide best solition. |
|
Ok, in the interest of moving forward:
I agree with @RobPavey that a single resave makes sense here to avoid lots of spurious diffs as development proceeds. I've gone through and renamed a few files and resaved them using the .prettierrc config as noted above. |
|
My old eyes agree with Brian (even though I have not yet contributed). But there should be consensus between this and the browser extension wikitree/wikitree-browser-extension#11 |
|
I agree with @bcaseyrls. |
As requested by here by @bcaseyrls I've prepared codestyle document and some formatting rules.
I've also moved
.mdfiles to/docsfolder (exceptREADME.md- this imho needs to be in the root)I'm not english native speaker (it's my second language) so there probably are some typos and grammar things that needs to be fixed, could you please check it?
There are few things that needs to be tweaked
Conflicting names(@bcaseyrls also opened the issue for that) where I've proposed 2 solutions.javascript modules) is obvious, but postponed for later (check discussion in the attached issue)naming convetion) is prefered for now, but it's rather my draft / wip and needs some discussionRendered
/docs/codestyle.md, for more comfortable reading.