Skip to content

♻️ Lint: include externs#26228

Merged
samouri merged 8 commits intoampproject:masterfrom
samouri:prettify-externs
Jan 22, 2020
Merged

♻️ Lint: include externs#26228
samouri merged 8 commits intoampproject:masterfrom
samouri:prettify-externs

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Jan 6, 2020 •
edited
Loading

summary
Extern javascript files should ideally be prettified as well. Currently I'm investigating the best way to go about this.

As brought up here: #26202 (comment)

interesting notes:

  • There was a duplicate typedef (GetCidDef). I deleted one of them and pointed the locations where it was used to the other definition.
  • We unfortunately cannot say "remove all rules" and so I've one by one disabled the ones that failed.

@samouri samouri mentioned this pull request Jan 6, 2020
Merged
@samouri samouri changed the title Lint: include externs ♻️ Lint: include externs Jan 6, 2020
rsimha
rsimha reviewed Jan 7, 2020
View reviewed changes
@samouri samouri marked this pull request as ready for review January 7, 2020 23:40
@amp-owners-bot amp-owners-bot bot requested a review from jeffjose January 7, 2020 23:40
rsimha
rsimha approved these changes Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

build-system/externs/amp.extern.js
* @typedef {{
* scope: string,
* createCookieIfNotPresent: (boolean|undefined),
* cookieName: (string|undefined),
Copy link
Copy Markdown
Contributor

@rsimha rsimha Jan 8, 2020

Choose a reason for hiding this comment

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

Nice catch. This PR is already showing its worth.

Copy link
Copy Markdown
Member Author

@samouri samouri Jan 8, 2020

Choose a reason for hiding this comment

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

Thanks :). I only caught this due to the rule against duplicate typedefs, but unfortunately I wasn't able to figure out how to keep it. Because while linting is run against all of the files at the same time, Closure Compiler evaluates many of the externs in isolation.

Copy link
Copy Markdown
Contributor

@lannka lannka Jan 14, 2020

Choose a reason for hiding this comment

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

I actually don't know why GetCidDef is here
@dvoytenko @erwinmombay do you recall? Is the cid API being called externally?

Copy link
Copy Markdown
Member Author

@samouri samouri Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@lannka is that a question we can reasonably defer to the future or a separate issue? (It seems unrelated to the purpose of this PR which is to make externs prettier)

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Jan 9, 2020

@jeffjose 🏓

@samouri samouri requested review from lannka and removed request for jeffjose January 14, 2020 20:34
@samouri samouri merged commit 54ed7fe into ampproject:master Jan 22, 2020
@samouri samouri deleted the prettify-externs branch January 22, 2020 22:23
@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jan 22, 2020

@samouri The default way to merge to master is via a squash commit. Any reason this was merged to master as 8 separate commits?

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Jan 22, 2020

@rsimha, my apologies! I'm not sure how I did that. It wasn't intentional / I'll double check my settings are correct before merging again

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jan 22, 2020

Another problem: master is red due to lint errors in extern files that were checked in since this PR was last tested a week ago.

Options to fix:

  1. Revert this PR, fix, and check in afresh so it's all in one commit.
  2. Check in a separate fix-only PR.

Given that this PR was initially merged as separate commits, I'd lean towards option 1.

FYI @rcebulko who is build cop this week.

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jan 22, 2020

@rsimha, my apologies! I'm not sure how I did that. It wasn't intentional / I'll double check my settings are correct before merging again

No worries. I'll see if there's a way we can prevent this from happening via an admin setting on our repo.

@samouri samouri mentioned this pull request Jan 22, 2020
Merged
@rcebulko
Copy link
Copy Markdown
Contributor

rcebulko commented Jan 22, 2020

I'm seeing this now, but it all happened so quickly it seems to already be resolved

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jan 22, 2020

No worries. I'll see if there's a way we can prevent this from happening via an admin setting on our repo.

Disabled via admin setting. Thanks @samouri for sending out the revert.

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

Reviewers

2 more reviewers

@lannka lannka lannka approved these changes

@rsimha rsimha rsimha approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes PR use: In Beta / Experimental

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@samouri @rsimha @rcebulko @lannka @googlebot @ampprojectbot