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

about_comments #19

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions general/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Guidelines and best practices used by Eventbrite to provide consistency and prev
0. [Conditionals](#conditionals)
0. [Assignments](#assignments)
0. [Naming Conventions](#naming-conventions)
0. [Comments](#comments)

## Conditionals

Expand Down Expand Up @@ -410,7 +411,53 @@ _superComplextName: function() {
UpdateAll: function() {
/ /code here
}
```
## comments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please capitalize here and below


### commented code

*We recommend not to leave commented code lingering around. If the code is meant to be used in the future, either save the code on a branch or in our internal paste bin via phabricator. (arc paste).*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we mention arc & phabricator since it's internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well phab is public. the product/software.


### when the comment has multiple lines we use the block form (i.e: JSDocs)
Copy link
Contributor

@BenAtEventbrite BenAtEventbrite Jul 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should read like:

### Multi-line comments

When the comment has multiple lines, we use the block for (i.e. JSDocs)

Is this saying that whenever we have a comment that spans multiple lines it has to be in JS doc form?

As in this:

/**
  */

vs.

/*
*/


```js

/**
* returns the calculated offset for lower bound range. (from)
* @example
* // returns 2
* scenario:
* Viewport Large
* skiping first and applying corrections
* @returns {integer} lower bound range
*/

const getMinBound = (pageSize = constants.DEFAULT_PAGE_SIZE) => (
pageSize === constants.DEFAULT_PAGE_SIZE ? constants.MIN_BOUND_SMALL : constants.MIN_BOUND_DEFAULT
);
```

### when the comment has a single line we instead one liner comments:
Copy link
Contributor

@BenAtEventbrite BenAtEventbrite Jul 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

### Single-line comments

When the comment has a single line, we instead use one-liner comments:


```js
const getToOffset = (pageNumber, middleRangeOffset, maxPagesShown, maxBound) => {
// default -> page is in the middle, and we have enough pages in both sides.
let to = pageNumber + middleRangeOffset;

// are we too close to the bottom? if so, we need to adjust the split
Copy link
Contributor

@BenAtEventbrite BenAtEventbrite Jul 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is on two lines but uses one-liner comments? Should it have used a different style?

// relative checks where the split doesn't work (sorry van damme)
if (to < maxPagesShown + 1) {
to = pageNumber + maxPagesShown;
}

// hard bound to the top
if (to > maxBound) {
// on small + 1
to = maxBound;
}

return to;
};
```

**[⬆ back to top](#table-of-contents)**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to mention about how inline comments are prohibited and why? You can also link to the eslint rule