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: remove extra forbid constraint #48

Merged
merged 10 commits into from
Jan 21, 2025

Conversation

suryaiyer95
Copy link
Collaborator

@suryaiyer95 suryaiyer95 commented Jan 21, 2025

Remove Extra.forbid constraint. It is not really required and is too strict. additionally Enum forbid is not supported in pydantic >= 2.0 <= 2.9

Added test environment for pydantic 2.8 and 2.10 also


Important

Remove Extra.forbid constraint from Pydantic models and update test environments for Pydantic 2.8 and 2.10.

  • Behavior:
    • Remove Extra.forbid constraint from Pydantic models in catalog.py and manifest.py.
    • Delete constants.py which defined Extra enum.
  • Testing:
    • Add test environments for Pydantic 2.8 and 2.10 in tox.ini and github-actions.yml.
    • Update test matrix in github-actions.yml to reflect new Pydantic versions.
  • Misc:
    • Remove unused Python 3.8 and PyPy 3.8 environments from tox.ini and github-actions.yml.

This description was created by Ellipsis for 1514bf1. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 831b509 in 30 seconds

More details
  • Looked at 233 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/schemas/catalog.py:12
  • Draft comment:
    Removing model_config with extra=Extra.forbid will allow extra fields in the data without raising an error. Ensure this change is intentional and aligns with the data validation requirements.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. src/datapilot/core/platforms/dbt/schemas/manifest.py:46
  • Draft comment:
    Removing model_config with extra=Extra.forbid will allow extra fields in the data without raising an error. Ensure this change is intentional and aligns with the data validation requirements.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_cFbsAuJt0xumqK2X


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 38dad6d in 42 seconds

More details
  • Looked at 32 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/schemas/manifest.py:49
  • Draft comment:
    The use of Optional[Optional[str]] is redundant. Simplify to Optional[str]. This pattern appears in multiple places in this file and in catalog.py.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_tZxoNpvHIsWxGwNZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 43a5d84 in 10 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. tox.ini:17
  • Draft comment:
    The py38 environment has been removed from the envlist. If this was intentional, please ensure that all dependencies and tests are compatible with the remaining environments. If not, consider adding it back.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR modifies the envlist in the tox.ini file to include pydantic28 and pydantic210 constraints. However, the py38 environment is removed without explanation, which might be unintentional.

Workflow ID: wflow_N8IzOLGhKcwnre2r


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on e409801 in 18 seconds

More details
  • Looked at 186 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/github-actions.yml:73
  • Draft comment:
    The PR description mentions removing an extra forbid constraint, but the changes in the PR are related to adding new test environments for different Python and Pydantic versions. This seems unrelated to the description.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_ruWyo7NjcS6VI2hR


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on cde2195 in 33 seconds

More details
  • Looked at 24 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. tox.ini:18
  • Draft comment:
    The PR title mentions removing an extra forbid constraint, but the changes here are about adding Python 3.8 and PyPy 3.8 to the test environments. Ensure the PR title accurately reflects the changes made.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_AjunABqFI3hCT5D4


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on f717673 in 42 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_8Fh4Ed4jcVOsNvIC


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

.github/workflows/github-actions.yml Outdated Show resolved Hide resolved
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 72d22ce in 21 seconds

More details
  • Looked at 47 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/github-actions.yml:265
  • Draft comment:
    The removal of the commented-out pypy3.8 environments is consistent with the PR title and does not affect the current workflow execution. Ensure that these environments are no longer needed before finalizing the removal.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The PR removes commented-out sections for pypy3.8 environments, which is consistent with the title 'remove extra forbid constraint'.

Workflow ID: wflow_p3YlDQyZFanzY90L


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on ffae1f8 in 33 seconds

More details
  • Looked at 428 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. tox.ini:17
  • Draft comment:
    Remove the py38 entry from the basepython section as Python 3.8 is no longer part of the test matrix.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_QgsXhVaKuG1OFzBr


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on e598b5e in 15 seconds

More details
  • Looked at 58 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/github-actions.yml:19
  • Draft comment:
    Removing comments that indicate the Python version is unnecessary and reduces readability. Consider keeping them for clarity. This applies to other similar removals in the file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The removal of comments is unnecessary and reduces code readability.

Workflow ID: wflow_q5eafWcPH7wjY1Sd


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 1514bf1 in 17 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. .github/workflows/github-actions.yml:177
  • Draft comment:
    The PR description mentions removing an extra forbid constraint, but the diff only shows a removal of a blank line. Please ensure that all intended changes are included in the commit.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_qWZlVi8l8NZwKU8P


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@anandgupta42 anandgupta42 self-requested a review January 21, 2025 21:22
@anandgupta42 anandgupta42 merged commit 78bd444 into main Jan 21, 2025
58 checks passed
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.

2 participants