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

feat(scripts): allow Prettier to format more files (YAML, Markdown) #103

Merged
merged 2 commits into from Oct 2, 2020

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Oct 1, 2020

We're not going to change the default globs in liferay-portal yet (or possibly ever?) because the Java Source Formatter currently handles these types.

However, we can make it easier for non-liferay-portal projects like liferay-learn that use liferay-npm-scripts to run Prettier over a broader range of files. It's a simple matter of expanding the list of allowed globs.

Test plan:

In order to actually test this I had these two PRs applied:

Then, with this npmscripts.config.js at the top level of the monorepo:

module.exports = {
    check: ['**/*.{md,ts,yml}'],
    fix: ['**/*.{md,ts,yml}'],
}

I created some bad files and ran
projects/npm-tools/packages/npm-scripts/bin/liferay-npm-scripts.js check and saw these errors:

x.md: BAD
x.ts: BAD
x.yml: BAD

and then ran check and saw them get fixed.

Then, for good measure, in a liferay-learn clone, I added "yml" and "md" to the globs in the npmscripts.config.js and repeated the test after a:

npm install
cp -R path/to/npm-scripts node_modules/@liferay/

ie.

npm run format:check

see:

site/homepage/x.md: BAD
site/homepage/x.yml: BAD
Prettier checked 21 files, 2 files have problems

Then:

npm run format

and see the errors fixed:

Prettier checked 21 files, fixed 2 files

Closes: #92

We're not going to change the default globs in liferay-portal yet (or
possibly ever?) because the Java Source Formatter currently handles
these types.

However, we can make it easier for non-liferay-portal projects like
liferay-learn:

https://github.com/liferay/liferay-learn

that use liferay-npm-scripts to run Prettier over a broader range of
files. It's a simple matter of expanding the list of allowed globs.

Test plan:

In order to actually test this I had these two PRs applied:

- #101
- #102

Then, with this npmscripts.config.js at the top level of the monorepo:

    module.exports = {
        check: ['**/*.{md,ts,yml}'],
        fix: ['**/*.{md,ts,yml}'],
    }

I created some bad files and ran
`projects/npm-tools/packages/npm-scripts/bin/liferay-npm-scripts.js
check` and saw these errors:

    x.md: BAD
    x.ts: BAD
    x.yml: BAD

and then ran `check` and saw them get fixed.

Then, for good measure, in a liferay-learn clone, I added "yml" and "md"
to the globs in the npmscripts.config.js and repeated the test after a:

    npm install
    cp -R path/to/npm-scripts node_modules/@liferay/

ie.

    npm run format:check

see:

    site/homepage/x.md: BAD
    site/homepage/x.yml: BAD
    Prettier checked 21 files, 2 files have problems

Then:

    npm run format

and see the errors fixed:

    Prettier checked 21 files, fixed 2 files

Closes: #92
@wincent
Copy link
Contributor Author

wincent commented Oct 1, 2020

cc @rdai10

Comment on lines +28 to +29
'.md',
'.markdown',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Frontend Infra repos we just use .md but there are some .markdown in liferay-portal.

Comment on lines +33 to +34
'.yml',
'.yaml',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likewise, we would use .yml, but I hear there are some .yaml out there too.

@wincent
Copy link
Contributor Author

wincent commented Oct 1, 2020

As I said here, I don't think these Windows failures are legit. Will retry them again later.

Seeing Windows fail strangely on these two PRs:

- https://github.com/liferay/liferay-frontend-projects/pull/101/checks?check_run_id=1197440869
- https://github.com/liferay/liferay-frontend-projects/pull/103/checks?check_run_id=1197441199

But not on a nearby run:

- https://github.com/liferay/liferay-frontend-projects/pull/102/checks?check_run_id=1194622874

I've retried a yesterday and today with no success, so in this commit I
just want to see if invalidating the cache does the trick.
@wincent
Copy link
Contributor Author

wincent commented Oct 2, 2020

See test PR showing what these changes look like when applied to liferay-learn: https://github.com/jrhoun/liferay-learn/pull/690

@wincent
Copy link
Contributor Author

wincent commented Oct 2, 2020

As this is basically a "config" change, I'm going to merge it. If anybody needs any follow-ups I will, er, follow-up.

wolf-pack

@wincent wincent merged commit 486c363 into master Oct 2, 2020
@wincent wincent deleted the wincent/yaml-etc branch October 2, 2020 07:10
wincent pushed a commit that referenced this pull request Dec 18, 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.

Allow npm-scripts to format .md and .yml files
1 participant