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

Resultset distinguishable #655

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Resultset distinguishable #655

merged 1 commit into from
Jun 28, 2023

Conversation

bbeat2782
Copy link

@bbeat2782 bbeat2782 commented Jun 23, 2023

Describe your changes

Adding description after html representation of ResultSet and error message for AttributeError when invalid operation is performed

Issue number

Closes #468

Checklist before requesting a review


📚 Documentation preview 📚: https://jupysql--655.org.readthedocs.build/en/655/

Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

overall, it looks good, can we make the font smaller? it's taking too much space

image

@bbeat2782
Copy link
Author

overall, it looks good, can we make the font smaller? it's taking too much space

image

Sure. Should I keep the ResultSet at the front as it is and reduce the size of text that comes after ResultSet or should I reduce all of them?

@edublancas
Copy link

please reduce all of them from ResultSet until PolarsDataFrame()

Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

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

lgtm. added some minor notes.

General thought:
image

I agree with @edublancas, I'm not sure the bottom message is clear enough. Maybe we can style it as the Truncated message (smaller font and italic style)?

src/tests/test_resultset.py Outdated Show resolved Hide resolved
src/sql/run.py Show resolved Hide resolved
Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

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

@AnirudhVIyer Also, please rebase.

src/sql/run.py Outdated Show resolved Hide resolved
src/sql/run.py Show resolved Hide resolved
@bbeat2782
Copy link
Author

Changes are

  1. Added text under ResultSet representation is now smaller and italicized
  2. Test function now tests different pandas-like functions with @pytest.mark.parametrize
  3. Use string formatting instead of + when adding 1)

If we want smaller text for 1), I will make the change.

src/sql/run.py Outdated Show resolved Hide resolved
Adding description after html representation of ResultSet and error message when invalid operation is performed

update changelog

update changelog for #468

making resultset distinguishable

update run with lint

modifying based on comments

smaller font and link to data frame

modify_resultset_test
Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

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

I think it looks good.

Why do we get this message here? is this the expected behavior?

image

@edublancas
I just noticed this message appears in our docs as well. What do you think about it?

image

@edublancas edublancas merged commit 84c464e into ploomber:master Jun 28, 2023
22 checks passed
@edublancas
Copy link

@yafimvo: good points. I'll open issues to follow up in your observations

tl3119 added a commit to tl3119/jupysql that referenced this pull request Jun 29, 2023
Making ResultSet distinguishable (ploomber#655)
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.

resultset are indistinguishable from pandas data frames
3 participants