Skip to content

Adds some comments and refactors some of the presubmit-check for clarity#616

Merged
erwinmombay merged 5 commits intoampproject:masterfrom
jaboc83:refactor-presubmit-checks
Oct 14, 2015
Merged

Adds some comments and refactors some of the presubmit-check for clarity#616
erwinmombay merged 5 commits intoampproject:masterfrom
jaboc83:refactor-presubmit-checks

Conversation

@jaboc83
Copy link
Copy Markdown
Contributor

@jaboc83 jaboc83 commented Oct 14, 2015

Some of the functions had missing or incorrect comments so this change adds
some more comments for consistency and makes a change to use a vinyl file
where it was specified in the comments but not used yet in the code. Also,
I renamed one of the functions to better reflect what was being tested (any vs all)
as it didn't make sense based on the current logic.

Some of the functions had missing or incorrect comments so this change adds
some more comments for consistency and makes a change to use a vinyl file
where it was specified in the comments but not used yet in the code. Also,
I renamed one of the functions to better reflect what was being tested (any vs all)
as it didn't make sense based on the current logic.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh can you change this to @return {boolean}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

@jaboc83
Copy link
Copy Markdown
Contributor Author

jaboc83 commented Oct 14, 2015

Bah! I need a line length checker setup in my vimrc...

@erwinmombay
Copy link
Copy Markdown
Member

@jaboc83 something like

set colorcolumn=80
highlight ColorColumn guibg=Gray14

should work (if you just wanted a ruler)

@jaboc83
Copy link
Copy Markdown
Contributor Author

jaboc83 commented Oct 14, 2015

Ha, yeah, I just added the exact same thing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sentence feels incomplete, maybe remove with ?

@erwinmombay
Copy link
Copy Markdown
Member

LGTM! thanks

erwinmombay added a commit that referenced this pull request Oct 14, 2015
Adds some comments and refactors some of the presubmit-check for clarity
@erwinmombay erwinmombay merged commit e1d3613 into ampproject:master Oct 14, 2015
@jaboc83 jaboc83 deleted the refactor-presubmit-checks branch October 14, 2015 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants