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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude webapp from spotless styling #16455

Merged
merged 5 commits into from
Sep 9, 2022

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Sep 8, 2022

What

This PR excludes the airbyte-webapp module from spotless styling. This module has its own auto-formatting plugins (e.g. prettier) and therefore does not also need spotless to run on it.

Having both auto-formatters running on this module can lead to inconsistencies and broken builds, such as the situation described in this PR: #16423

How

Excludes the airbyte-webapp module from the spotless target in the top-level build.gradle.

馃毃 User Impact 馃毃

No user impact.

@lmossman lmossman marked this pull request as ready for review September 8, 2022 17:48
@lmossman lmossman temporarily deployed to more-secrets September 8, 2022 17:48 Inactive
@lmossman lmossman requested review from krishnaglick and a team September 8, 2022 17:48
Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code LGTM

@lmossman lmossman temporarily deployed to more-secrets September 8, 2022 18:06 Inactive
@krishnaglick
Copy link
Contributor

eslint and prettier are only configured in airbyte-webapp. We should copy those over. Or symlink them?

@lmossman
Copy link
Contributor Author

lmossman commented Sep 9, 2022

@krishnaglick it looks like we already have some cypress-specific eslint configuration in airbyte-webapp-e2e-tests:

"eslintConfig": {
"env": {
"browser": true,
"node": true
},
"rules": {
"cypress/no-unnecessary-waiting": "warn"
},
"extends": [
"plugin:cypress/recommended"
]
},

So I don't think we need to add more eslint configuration there. But I have added prettier to that module and symlinked the .prettierrc.js files as you recommended, and I ran prettier on that module and committed the changes to this PR

@lmossman lmossman temporarily deployed to more-secrets September 9, 2022 17:55 Inactive
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

LGTM!

@lmossman lmossman merged commit 787e87d into master Sep 9, 2022
@lmossman lmossman deleted the lmossman/exclude-webapp-from-spotless branch September 9, 2022 19:19
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* exclude webapp from spotless styling

* also exclude airbyte-webapp-e2e-tests

* add comma

* add prettier to airbyte-webapp-e2e-tests

* run prettier on airbyte-webapp-e2e-tests
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* exclude webapp from spotless styling

* also exclude airbyte-webapp-e2e-tests

* add comma

* add prettier to airbyte-webapp-e2e-tests

* run prettier on airbyte-webapp-e2e-tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants