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

Stop formatting python with spotless #3388

Merged
merged 11 commits into from
May 14, 2021
Merged

Stop formatting python with spotless #3388

merged 11 commits into from
May 14, 2021

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented May 13, 2021 •
edited
Loading

What

  • Currently when you run an integration test for a connector it blows up the diff for python, even though we've made a lot of attempts to prevent this. Fundamentally the problem here is we have 2 formatters trying to format the same files and this is causing a headache.

How

  • Stop using spotless for python. Just use the existing formatters (e.g. black, etc) in airbyte-python.
  • That leaves one gap, which is injecting licenses. So this PR adds a new tool that just does licenses.
  • This should allow us to stop having awful diffs in python projects when we run spotlessApply.

How to read the PR

  • Just look at files that end in *.gradle.
  • Everything else is just applying a license. The diff is so big because this license provider is adding an extra new line at the beginning and end of licenses.

@cgardens cgardens changed the title Remove build dependency on all python modules Stop formatting python with spotless May 13, 2021
@cgardens cgardens requested a review from jrhizor May 13, 2021 06:50
cgardens
cgardens commented May 13, 2021
View reviewed changes
airbyte-cdk/python/build.gradle Outdated
rootProject.build.dependsOn generateProtocolClassFiles
installReqs.dependsOn(generateProtocolClassFiles)
task generateProtocolClassFiles(dependsOn: [generateProtocolClassFilesWithoutLicense])
blackFormat.dependsOn generateProtocolClassFilesWithoutLicense
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this makes sense. 🤞 in other words any styling tasks have to happen after any generation tasks.

cgardens
cgardens commented May 13, 2021
View reviewed changes
build.gradle
source = fileTree(dir: projectDir)
.include("**/*.py")
.exclude(".venv/**/*.py")
.exclude("**/__init__.py")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maintaining existing behavior where we do license init files. because they are blank it seems to cause havoc on these license tools.

sherifnada
sherifnada approved these changes May 13, 2021
View reviewed changes
airbyte-cdk/python/build.gradle Outdated
rootProject.spotlessPython.mustRunAfter(generateProtocolClassFilesWithoutLicense)
rootProject.build.dependsOn generateProtocolClassFiles
installReqs.dependsOn(generateProtocolClassFiles)
task generateProtocolClassFiles(dependsOn: [generateProtocolClassFilesWithoutLicense])
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need the distinction between with/without license then? seems like nothing depends on generateProtocolClassFiles

@cgardens cgardens merged commit 0df5317 into master May 14, 2021
@cgardens cgardens deleted the cgardens/style_fest branch May 14, 2021 00:46
htrueman added a commit that referenced this pull request May 18, 2021
@htrueman @davinchia
@yevhenii-ldv @vitaliizazmic @cgardens @avaidyanatha @jrhizor @vovavovavovavova @marcosmarxm @jamakase @masonwheeler @ChristopheDuong @sherifnada @michel-tricot
Source Iterable: update to use latest version of CDK (#3378)
588553e 
* Requirements updated to CDK.
airbyte-protocol and base-python requirements removed.

* Bugfix: BufferedStreamConsumer. (#3387)

* Format.

* Bump versions.

* main_dev.py renamed to main.py
README.md updated

* Source Stripe: Add Acceptance Tests to Stripe Connector (#3367)

* Add Acceptance Tests to Stripe Connector

* move configured_catalog.json to sample_files

* bump version

Co-authored-by: ykurochkin <y.kurochkin@zazmic.com>

* Legacy lib references removed

* FB Marketing source - lookback window logic not functioning correctly

* FB Marketing source #1390 - returning buffered record while incremental sync

* FB Marketing source #1390 - improving checking while syncing buffered record

* FB Marketing source #1390 - adding loop_back to IncrementalStreamAPI

* FB Marketing source #1390 - bump version

* FB Marketing source #1390 - add CHANGELOG.md

* Stop formatting python with spotless (#3388)

* add test that migration output schema same as source schema (#3356)

* Add updated architecture diagram to high level docs. (#3399)

* Add updated architecture doc to high level docs.

* Address review comments

Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local>

* Correct GA readme error. (#3407)

* make shopify more resilient to timeouts (#3409)

* Update migration schema to include recent changes to the StandardSync object. (#3414)

* Update all of Pydantic to 1.6.2 per Dependabot. (#3408)

* Update all to 1.6.2.

* Publish new airbyte-cdk version.

* Use repr instead of str for exceptions.

* Use rc.

* Edit test.

* Bump for SAT.

* Format.

* Docker ignore update. Fix setup.py

* fixing ONLY problematic fields in freshdesk JSON schemas (#3376)

* bump airbyte-webapp version (#2266)

* add configuration for bumping webapp versionn

* set to current version

* Bump version: 0.16.0-alpha → 0.16.1-alpha

* Revert "Bump version: 0.16.0-alpha → 0.16.1-alpha"

Thiss reverts commit fdbf6dc.

* also update package lock so we don't run into files changed errors

* use 0.19.0-alpha

* add npm webapp version

* Add a CDK speedrun tutorial doc (#3403)

* Add CDK Speedrun document.

* Finish speedrun doc.

* Address review comments

* Add to SUMMARY.md

Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local>

* Add Rust as a connector specific dependency to source-file (#3426)

* Add Rust as a connector specific dependency to source-file

* Add more details about installation.

* Markdown lines are weird.

Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local>

* API update to latest airbyte-cdk version

* Add section Deploy Local on Windows (#3425)

* add deploy on windows steps

* correct minor

* change suggestions by @avaidyanatha

* GitBook: [master] 161 pages and 75 assets modified

* Display icons (#3140)

* Display icons

* Improve icons views

* MS SQL Server Destination implementation

Fixes issue #613.

Normalization is not yet enabled.  This will have to be added at a later point.

* Workflow to handle operations (custom transformation) (#3379)

* Keep normalization backward compatible with old settings from destination

* Bumpversion normalization image

* add npm install before all npm run generates' (#3442)

* restart containers if they fail automatically (#3423)

* Update link for contribution scheduling (#3443)

* Address issue with icon in onboarding (#3437)

* rename toy connector tutorial to "Build a connector the hard way"  (#3421)

* Upload test reports (from integration test slash commands) as GitHub artifacts (#3416)

* Archive test reports in github workflow

* Archive Test reports only when failures

* Fixing SqlServerOperations.java (#3454)

Fixing some issues with `SqlServerOperations`, which was out of sync with recent changes to `SqlOperations`.

* Add redirect to cdk tutorial page (#3456)

* add redirect to cdk tutorial page

* change path to cdk README.md

Co-authored-by: Davin Chia <davinchia@gmail.com>
Co-authored-by: Yevhenii <34103125+yevhenii-ldv@users.noreply.github.com>
Co-authored-by: ykurochkin <y.kurochkin@zazmic.com>
Co-authored-by: vitaliizazmic <75620293+vitaliizazmic@users.noreply.github.com>
Co-authored-by: Charles <giardina.charles@gmail.com>
Co-authored-by: Abhi Vaidyanatha <abhi@airbyte.io>
Co-authored-by: Abhi Vaidyanatha <abhivaidyanatha@Abhis-MacBook-Pro.local>
Co-authored-by: Jared Rhizor <jared@dataline.io>
Co-authored-by: vovavovavovavova <39351371+vovavovavovavova@users.noreply.github.com>
Co-authored-by: Marcos Marx <marcosmarxm@users.noreply.github.com>
Co-authored-by: Marcos Marx <marcos@airbyte.io>
Co-authored-by: Artem Astapenko <3767150+Jamakase@users.noreply.github.com>
Co-authored-by: masonwheeler <masonwheeler@yahoo.com>
Co-authored-by: Christophe Duong <christophe.duong@gmail.com>
Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
Co-authored-by: Michel Tricot <michel@dataline.io>
@maxi297 maxi297 mentioned this pull request Dec 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherifnada sherifnada sherifnada approved these changes

@jrhizor jrhizor Awaiting requested review from jrhizor

@davinchia davinchia Awaiting requested review from davinchia

@ChristopheDuong ChristopheDuong Awaiting requested review from ChristopheDuong

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cgardens @sherifnada