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.

Sign up for GitHub

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

Jump to bottom

Allow npm-scripts to format .md and .yml files #92

Closed
wincent opened this issue Sep 30, 2020 · 2 comments · Fixed by #103
Closed

Allow npm-scripts to format .md and .yml files #92

wincent opened this issue Sep 30, 2020 · 2 comments · Fixed by #103
Labels
feat npm-scripts npm-tools

Comments

@wincent
Copy link
Contributor

wincent commented Sep 30, 2020 •
edited
Loading

Issue type

  • 🎁 Feature request

Description

Desired behavior:

As per #91 — they want to format YAML and Markdown too using @liferay/npm-scripts.

Current behavior:

We only support a hard-coded list of file extensions for the reasons discussed in the linked issue.
@wincent wincent mentioned this issue Sep 30, 2020
Closed
@jbalsas
Copy link
Contributor

jbalsas commented Sep 30, 2020

Just quick comment, in liferay-portal we have

  • Lots of .md files
  • Few .yml files
  • Several .yaml files

I'm assuming we don't want to format those in the liferay-portal repo but allow them to be formatted through @liferay/npm-scripts in other projects?

@wincent
Copy link
Contributor Author

wincent commented Sep 30, 2020

Correct. My idea is to allow them to be formatted, but not to change the default globs in liferay-portal itself (because they are already handled there by the Java Source Formatter; we could look at handing over responsibility for those filetypes to Prettier in the future, but that would be a separate thing).

This was referenced Oct 1, 2020
Merged
Merged
wincent added a commit that referenced this issue Oct 1, 2020
@wincent
fix(scripts): apply Liferay-specific Prettier overrides to TS also
9b91ba6 
Noticed while testing a PR for:

#92

that we aren't applying our Liferay-specific Prettier overrides to TS
files. ie. we're not enforcing this:

    }
    else {

Test plan:

Add a ".ts" file, "x.ts" with these contents:

    if (true) {
    alert('t');
    } else {
        alert('f');
    }

Add an "npmscripts.config.js" file (you don't need the ".md" and ".yml"
in there, but this is the file I had to create to test the other PR
anyway, so I'm just using the same one):

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

Run `projects/npm-tools/packages/npm-scripts/bin/liferay-npm-scripts.js
check` and see the error reported.

Run `projects/npm-tools/packages/npm-scripts/bin/liferay-npm-scripts.js
fix` and see the code fixed to:

    if (true) {
            alert('t');
    }
    else {
            alert('f');
    }
wincent added a commit that referenced this issue Oct 1, 2020
@wincent
fix(scripts): apply Liferay-specific Prettier overrides to TS also
2808fa3 
Noticed while testing a PR for:

#92

that we aren't applying our Liferay-specific Prettier overrides to TS
files. ie. we're not enforcing this:

    }
    else {

Test plan:

Add a ".ts" file, "x.ts" with these contents:

    if (true) {
    alert('t');
    } else {
        alert('f');
    }

Add an "npmscripts.config.js" file (you don't need the ".md" and ".yml"
in there, but this is the file I had to create to test the other PR
anyway, so I'm just using the same one):

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

Run `projects/npm-tools/packages/npm-scripts/bin/liferay-npm-scripts.js
check` and see the error reported.

Run `projects/npm-tools/packages/npm-scripts/bin/liferay-npm-scripts.js
fix` and see the code fixed to:

    if (true) {
            alert('t');
    }
    else {
            alert('f');
    }
@wincent wincent mentioned this issue Oct 1, 2020
Merged
wincent added a commit that referenced this issue Oct 1, 2020
@wincent
chore: update ESLint to latest v7 release
e8ab576 
May as well do this now, as I found it easier to test my solution for:

#92

with only one version of ESLint in the monorepo (previously we had one
version in npm-scripts and another at the top level).

Closes: #47
@wincent wincent mentioned this issue Oct 1, 2020
Merged
wincent added a commit that referenced this issue Oct 1, 2020
@wincent
feat(scripts): allow Prettier to format more files (YAML, Markdown)
33b18c7 
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 wincent mentioned this issue Oct 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feat npm-scripts npm-tools
Projects
None yet
Milestone
No milestone
2 participants
@wincent @jbalsas