这是indexloc提供的服务,不要输入任何密码
Skip to content

🏗 Pre-emptive lint fixes + clean up #30700

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

Merged
merged 4 commits into from
Oct 16, 2020
Merged

🏗 Pre-emptive lint fixes + clean up #30700

merged 4 commits into from
Oct 16, 2020

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Oct 16, 2020

Yarn can sometimes incorrectly resolve caret semver versions in its lockfile (typically during upgrades). E.g. prettier version ^2.0.0 is resolved to 2.0.1 instead of 2.1.2 in yarn.lock (search for "prettier@^2.0.0:"). NPM correctly resolves caret versions, and doesn't have this bug.

This PR contains a couple of pre-emptive fixes that will prevent a bunch of new Prettier errors from appearing when we switch from Yarn to NPM:

  1. Updates the default globs used by gulp lint to explicitly ignore files in .gitignore
  2. Uses the exact same prettier version for gulp lint and gulp prettify

With this PR, the switch from Yarn to NPM should be free of any new linting errors.

Unblocks #30694

@rsimha rsimha self-assigned this Oct 16, 2020
@google-cla google-cla bot added the cla: yes label Oct 16, 2020
@rsimha rsimha requested a review from morsssss October 16, 2020 14:54
@rsimha
Copy link
Contributor Author

rsimha commented Oct 16, 2020

@morsssss to approve doc changes.

Copy link
Contributor

@morsssss morsssss left a comment

Choose a reason for hiding this comment

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

To me at least, in general the HTML that was changed here looked better before these linting changes. In some cases, there were ellipses indicating more HTML would be used in the real world... I also prefer that templates be on their own line, not sandwiched between <template> and </template> on a single line.

What do you think? Do you prefer the new layouts? Or are these simply a result of standardizing linting?

In a couple of cases, the <template> tags did seem to confuse the linter.

@amp-owners-bot
Copy link

Hey @gmajoulet, @newmuis, @Enriqe! These files were changed:

extensions/amp-story/amp-story-bookend.md
extensions/amp-story/amp-story.md

Copy link
Contributor Author

@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.

What do you think? Do you prefer the new layouts? Or are these simply a result of standardizing linting?

@morsssss This is the eternal battle between flexibility and standardization that Prettier makes us wage 😃

From a technical standpoint, Prettier isn't making these decisions, it's merely using the underlying markdown formatting standard, which now prescribes unwrapping multi-line blocks that can fit on a single line. Personally, I'm not against the unwrapping because there are already several similar instances across our code base (example). I can add manual overrides to prevent these <template> blocks from being unwrapped, but it will introduce a maintenance burden on MD authors, and potentially result in a loss of standardization in future.

I'm happy to defer to you on this one. Let me know what you think!

@rsimha
Copy link
Contributor Author

rsimha commented Oct 16, 2020

After chatting offline with @morsssss, we decided to disable all automatic language-specific formatting inside MD files. As a result, this PR no longer contains any of the unwrapping it earlier did.

@rsimha rsimha dismissed morsssss’s stale review October 16, 2020 20:18

These files have been reverted.

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

One small question, otherwise looks great.

@morsssss
Copy link
Contributor

thanks @rsimha !

@rsimha rsimha merged commit f0aeae9 into ampproject:master Oct 16, 2020
@rsimha rsimha deleted the 2020-10-16-PrettierFixes branch October 16, 2020 20:58
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants