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

Support new version of WP and update dependencies #101

Merged
merged 20 commits into from
Dec 7, 2023

Conversation

hueitan
Copy link
Member

@hueitan hueitan commented Nov 21, 2023

  • Modernize build dependencies wikipedia-preview#177
  • shell script in scripts folder
  • update package.json dependencies version
  • update composer.json require-dev version
  • update version to 1.12.0
  • replace circleci with github action
  • stop circleci running (need admin persmission)
  • npm link test well

scripts/postinstall.sh Outdated Show resolved Hide resolved
@hueitan hueitan changed the title Support new version of WP Support new version of WP and update dependencies Nov 22, 2023
@stephanebisson
Copy link
Contributor

Most (or all) js files in this PR remove spaces insides parenthesis, which goes against WMF standards. I'm wondering why this is happening now. Have wordpress standards changed? Should we include eslint-wikimedia preset?

@medied
Copy link
Contributor

medied commented Dec 6, 2023

Most (or all) js files in this PR remove spaces insides parenthesis, which goes against WMF standards. I'm wondering why this is happening now. Have wordpress standards changed? Should we include eslint-wikimedia preset?

That's a good question. I couldn't find the exact pinpoint in a changelog where the change is reflected, but it looks like @wordpress/eslint-plugin/recommended does remove spacing by default so I suppose Wordpress standards did change. However there is also a @wordpress/eslint-plugin/recommended-with-formatting that does preserve the spacing, after testing locally:

...
  35:3   error  There must be a space before this paren  space-in-parens
  36:14  error  There must be a space after this paren   space-in-parens
  36:15  error  There must be a space after this paren   space-in-parens
  36:24  error  There must be a space before this paren  space-in-parens
  38:15  error  There must be a space after this paren   space-in-parens
  38:23  error  There must be a space before this paren  space-in-parens
  39:13  error  There must be a space after this paren   space-in-parens
  39:27  error  There must be a space before this paren  space-in-parens
  39:37  error  There must be a space after this paren   space-in-parens
  39:55  error  There must be a space before this paren  space-in-parens
  42:3   error  There must be a space before this paren  space-in-parens
  43:2   error  There must be a space before this paren  space-in-parens
  43:3   error  There must be a space after this paren   space-in-parens
  43:39  error  There must be a space before this paren  space-in-parens

✖ 838 problems (834 errors, 4 warnings)
  831 errors and 0 warnings potentially fixable with the `--fix` option.

More info here: https://developer.wordpress.org/block-editor/reference-guides/packages/packages-eslint-plugin/#usage and here https://wordpress.org/support/topic/eslint-pluginwordpress-eslint-plugin-not-settings-spaces/

I think I'm leaning towards keeping the spaces just to simplify this PR. Thoughts?

@stephanebisson
Copy link
Contributor

[...]
I think I'm leaning towards keeping the spaces just to simplify this PR. Thoughts?

I agree and I prefer to stay closer to WMF standards than Wordpress standards when they conflict.

@medied medied marked this pull request as ready for review December 6, 2023 19:50
@stephanebisson stephanebisson merged commit 69cd32e into main Dec 7, 2023
1 check passed
@stephanebisson stephanebisson deleted the support-new-wp-version branch December 7, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants