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

fix client.insert_dataframe() for tuple columns in tables #425

Closed

Conversation

stankudrow
Copy link

@stankudrow stankudrow commented Mar 12, 2024

Notes:

  • This PR

    • Some of the changed files initially had minor changes, but after passing them through black/flake8/ruff packages, "cosmetic" changes grew dramatically - sorry for overburdening the PR. However, it may be seen as a shy step to a unified approach on code formatting (to be refined in the following PRs) and adopt black/mypy/ruff and other tools to assure a uniform and ubiquitous approach on the code quality.
    • Drifting towards the f-string formatting seems important, but in this PR it is done partially.
  • To do:

    • It would be nice to drop the setup.py support in favour of the pyproject.toml file (a historical document is PEP-621) and modern packaging and dependency managers like hatch, poetry, pdm and so forth. I am willing to do this work in one of the next PRs.
    • According to the status of Python versions, ensuring "python>=3.9" will do (easily achieved with pyproject.toml and a Python PD manager, clarifying the previous point)

The list of selected packages from the virtual environment:

  • black : 24.2.0
  • clickhouse-cityhash : 1.0.2.4
  • clickhouse-driver : 0.2.7
  • Cython : 3.0.9
  • flake8 : 7.0.0
  • freezegun : 1.4.0
  • lz4 : 4.3.3
  • mypy : 1.9.0
  • numpy : 1.26.4
  • pandas : 2.2.1
  • parametrized : 66.0.2 (there is the latest 66.0.3 -> to be removed in favour of pytest.mark.parametrize)
  • pytest : 7.4.4
  • ruff : 0.3.2
  • zstd : 1.5.5.1

Checklist:

  • Add tests that demonstrate the correct behaviour of the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Ensure PR doesn't contain untouched code reformatting: spaces, etc. -> the changed modules have been passed multiple times through the black and ruff packages;
  • Run flake8 and fix issues -> issues fixed, after moving to the pyproject.toml support the linters and any other code-quality packages (a good idea is to adopt the black+mypy+ruff as a dev-qa core) will be tuned in a more explicit way;
  • Run pytest no tests failed. See the dev docs -> a test case within the pandas related test suite has been added.

@xzkostyan
Copy link
Member

Ensure PR doesn't contain untouched code reformatting: spaces, etc. -> the changed modules have been passed multiple times through the black and ruff packages;

Black or ruff linters should be added in different PR. I don't see any profit from passing code through these linters without any validation in actions.

Drifting towards the f-string formatting seems important, but in this PR it is done partially.

This also should be done in different PR.

This PR should contain only minor changes about insert_dataframe.

@stankudrow
Copy link
Author

Fair enough, easier or cheaper to close this PR in favour of #426.

@stankudrow stankudrow closed this Mar 12, 2024
@stankudrow stankudrow deleted the fix-insert-tuple-error-pr417 branch March 12, 2024 20:44
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.

Problems while inserting into a table with a Tuple column
2 participants