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

Accept Pandas Series in lov property #1447

Merged
merged 6 commits into from
Jul 16, 2024

Conversation

Satoshi-Sh
Copy link
Contributor

@Satoshi-Sh Satoshi-Sh commented Jun 25, 2024

Related Issue

Fixes #1444

Updates

  • Add a condition to convert a pandas.Series to a list in the _adapter.py
  • Add a new test case to test_selector.py

Comments

I made a new test case for pd.Series but it doesn't pass. Can you have a look at it?

Screenshot from 2024-06-25 05-19-15

@FlorianJacta
Copy link
Member

This issue is, in fact, still being discussed with the R&D. We will get back to you when the R&D agrees

@Satoshi-Sh
Copy link
Contributor Author

Got it. Is it better to close this or fine to keep this as is? @FlorianJacta

@FlorianJacta
Copy link
Member

Let's keep it for the moment

@Satoshi-Sh Satoshi-Sh force-pushed the feat/#1444/lov-pandas-series branch from 39be3cd to faac65e Compare July 12, 2024 09:26
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

simple and efficient
Could you update your branch ?

@Satoshi-Sh Satoshi-Sh force-pushed the feat/#1444/lov-pandas-series branch from faac65e to 0a767c6 Compare July 12, 2024 09:35
@Satoshi-Sh Satoshi-Sh marked this pull request as ready for review July 12, 2024 09:49
@Satoshi-Sh
Copy link
Contributor Author

It's updated but the test I wrote fails for some reason. Can you have a look at it?

Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

you also need to manage the type in builder.py (for the default value) in def __get_list_of()
I would recommend a generic approach

if not isinstance(lof, list) and hasattr(lof, "tolist"):
            try:
                return lof.tolist()  #type: ignore[union-attr]
            except Exception as e:
                _warn("Error accessing List of values", e)

tests/gui/builder/control/test_selector.py Outdated Show resolved Hide resolved
@FredLL-Avaiga
Copy link
Member

If you update your branch and resolve the conversations, I think we can merge this branch.

@Satoshi-Sh Satoshi-Sh force-pushed the feat/#1444/lov-pandas-series branch from b38d284 to 791d516 Compare July 15, 2024 17:11
@Satoshi-Sh
Copy link
Contributor Author

@FredLL-Avaiga Resolved conversations and updated the branch.

@FredLL-Avaiga FredLL-Avaiga self-requested a review July 15, 2024 17:48
FredLL-Avaiga
FredLL-Avaiga previously approved these changes Jul 15, 2024
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

I'd like @FabienLelaquais and @dinhlongviolin1 to take a look

@FabienLelaquais
Copy link
Member

d with the R&D. We will get back to you when the R&D agrees

On it, @FredLL-Avaiga

Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

This is really simple and clear.
I added a comment just in case. Do it or not. Let me know.
No problem to approve+merge then.

tests/gui/builder/control/test_selector.py Outdated Show resolved Hide resolved
@FredLL-Avaiga FredLL-Avaiga self-requested a review July 15, 2024 19:12
Copy link
Member

@FabienLelaquais FabienLelaquais left a comment

Choose a reason for hiding this comment

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

👌

@FredLL-Avaiga FredLL-Avaiga merged commit a0ddd41 into Avaiga:develop Jul 16, 2024
155 of 156 checks passed
@FredLL-Avaiga
Copy link
Member

Thank you @Satoshi-Sh

@Satoshi-Sh
Copy link
Contributor Author

Thanks for your help, Fred 🚀

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.

Accept Pandas Series in lov property
4 participants