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

feature/#398-expand-exposed-type-parameter #2296

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

sohamkumar05
Copy link

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

We are also including pandas.DataFrame and numpy.ndarray as exposed type.

Related Tickets & Documents

How to reproduce the issue

Checklist

We encourage you to keep the code coverage percentage at 80% and above.

  • Does this solution meet the acceptance criteria of the related issue?
  • Is the related issue checklist completed?
  • Does this PR adds unit tests for the developed code? If not, why?
  • End-to-End tests have been added or updated?
  • Was the documentation updated, or a dedicated issue for documentation created? (If applicable)
  • Is the release notes updated? (If applicable)

@sohamkumar05 sohamkumar05 marked this pull request as draft December 2, 2024 06:25
@sohamkumar05 sohamkumar05 marked this pull request as ready for review December 2, 2024 06:45
@sohamkumar05 sohamkumar05 changed the title made changes to excel and csv data node feature/#398-expand-exposed-type-parameter Dec 2, 2024
@jrobinAV jrobinAV added Core Related to Taipy Core Core: Data node 🟨 Priority: Medium Not blocking but should be addressed labels Dec 2, 2024
Copy link
Member

@trgiangdo trgiangdo left a comment

Choose a reason for hiding this comment

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

Thank you @sohamkumar05 for your contribution with the Pull Request.

I believe the PR is in a good shape. There are a few refinements needed, and then the Taipy team will do some testing before we can merge your PR.

taipy/core/data/_tabular_datanode_mixin.py Outdated Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
git Outdated Show resolved Hide resolved
taipy/core/config/data_node_config.py Outdated Show resolved Hide resolved
@jrobinAV jrobinAV requested a review from trgiangdo December 5, 2024 15:26
@trgiangdo trgiangdo self-assigned this Dec 12, 2024
Copy link
Member

@trgiangdo trgiangdo left a comment

Choose a reason for hiding this comment

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

The PR looks good to me. I believe it's almost ready to be merged.

I have 1 more request just to make sure this works properly with Taipy serialization.

Can you write a few test cases exploring cases where instead of import pandas as pd and use pd.DataFrame as the exposed_type, do:

  • import pandas and use pandas.DataFrame
  • from pandas import DataFrame and use just the DataFrame
  • from pandas import DataFrame as DF and use DF

Similarly, try different imports for np.ndarray

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core: Data node Core Related to Taipy Core 🟨 Priority: Medium Not blocking but should be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand the exposed_type parameter of DataNode to accept actual Python types
3 participants