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

230 customizing results output #274

Merged

Conversation

AnirudhVIyer
Copy link

@AnirudhVIyer AnirudhVIyer commented Mar 20, 2023

Enabled urls in ResultSet output and pandas DataFrame to be clickable

Issue ticket number and link

Closes #230

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added thorough tests (when necessary).
  • I have added the right documentation in the docstring and changelog (when needed)

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

@edublancas
Copy link

is this ready for review? looks like you ran some markdown formatted cause the CHANGELOG was edited

@AnirudhVIyer
Copy link
Author

Yeah, it is ready for review.

@AnirudhVIyer AnirudhVIyer marked this pull request as ready for review March 20, 2023 01:38
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.

please check your formatting settings, I see a lot of changes in files that should not be edited (example, the CHANGELOG and some test files)

@AnirudhVIyer
Copy link
Author

Will do that.

@AnirudhVIyer
Copy link
Author

I have added a new table with urls in the conftest file to run a few tests. Is it alright?

@idomic
Copy link

idomic commented Mar 20, 2023

I have added a new table with urls in the conftest file to run a few tests. Is it alright?

If you can't use/utilize the existing fixtures then it's fine.

@AnirudhVIyer
Copy link
Author

Hi @edublancas can you approve the workflow checks. Thank You.

@idomic
Copy link

idomic commented Mar 20, 2023

@AnirudhVIyer please make sure you've added the issue number it closes in the PR description.

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, just added one minor comment.

I had an issue reproducing it for pandas data frames, am I doing something wrong?

image

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

overall it looks good, just added one minor comment.

I had an issue reproducing it for pandas data frames, am I doing something wrong?

image

Hi Eduardo, to make the URLs clickable in a dataframe, we need to add a style function that returns a Styler object, not the original dataframe. If we return the Styler object, users won't be able to conduct further analysis on it. Therefore, I added a styling function to the returned dataframe as an attribute. Users can use this attribute to display clickable links when they want to but still retain the original dataframe for further use.

you can display the dataframe with styling like this:

df.styled_func()

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.

ok, since getting this to work with pandas involves users knowing a bit of the implementation details, I think it's better to only keep the URL parsing the ResultSet - it's ok if the links go away when turning it into a pandas data frame

@AnirudhVIyer
Copy link
Author

ok, since getting this to work with pandas involves users knowing a bit of the implementation details, I think it's better to only keep the URL parsing the ResultSet - it's ok if the links go away when turning it into a pandas data frame

Sure, I will make the required changes.

@AnirudhVIyer
Copy link
Author

I have removed the pandas url attribute.

@edublancas edublancas merged commit e33838c into ploomber:master Mar 22, 2023
@edublancas
Copy link

great work!

@AnirudhVIyer
Copy link
Author

thank you!

@idomic
Copy link

idomic commented Mar 27, 2023

Thanks for committing @AnirudhVIyer !
If you're up for it, this is another good first issue you can tackle!

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.

customizing results output
3 participants