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

🧹 refurb cleanup + one typo #152

Merged
merged 3 commits into from
Dec 27, 2022
Merged

Conversation

jayceslesar
Copy link
Contributor

Used the python library refurb against this repo and added most of the suggested changes.

Feel free to counteract any!

I think good future work will be dropping support for anything lower than python 3.9 as well and porting all annotations to future syntax in terms of codebase wide changes.

@jonasvdd
Copy link
Member

jonasvdd commented Dec 25, 2022

Hi @jayceslesar! Really cool repo! Had not heard from it yet!

Based on these statistics, python 3.8 is still on the rise.
However, as the older versions would still be supported by e.g., plotly-resampler<=0.8.X, there is a case for being more severe with our version support.

What would be the main advantage of porting these annotations @jayceslesar?
What are your thoughts on this @jvdd.

I will review your code somewhere this week!

And of course,
Happy holidays! 🎅🏼

@jonasvdd jonasvdd requested review from jonasvdd and jvdd December 25, 2022 16:42
@jonasvdd jonasvdd added the enhancement New feature or request label Dec 25, 2022
Copy link
Member

@jonasvdd jonasvdd left a comment

Choose a reason for hiding this comment

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

LGTM!

Certainly an improvement of the code quality! 👏🏼
Can you give some feedback @jvdd?

@jayceslesar
Copy link
Contributor Author

Hi @jayceslesar! Really cool repo! Had not heard from it yet!

Based on these statistics, python 3.8 is still on the rise. However, as the older versions would still be supported by e.g., plotly-resampler<=0.8.X, there is a case for being more severe with our version support.

What would be the main advantage of porting these annotations @jayceslesar? What are your thoughts on this @jvdd.

I will review your code somewhere this week!

And of course, Happy holidays! 🎅🏼

I guess I am biased that the new annotations are really just nicer to look at haha, but I see your points, no reason to not support older versions if they are still being widely used

And cheers! Happy holidays

Copy link
Member

@jvdd jvdd left a comment

Choose a reason for hiding this comment

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

LGTM! Applied some minor changes myself :)

@jvdd jvdd changed the title refurb cleanup + one typo 🧹 refurb cleanup + one typo Dec 27, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #152 (949b445) into main (46499ef) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
+ Coverage   97.08%   97.30%   +0.22%     
==========================================
  Files          12       12              
  Lines         892      891       -1     
==========================================
+ Hits          866      867       +1     
+ Misses         26       24       -2     
Impacted Files Coverage Δ
plotly_resampler/aggregation/aggregators.py 91.66% <ø> (ø)
plotly_resampler/__init__.py 84.61% <100.00%> (+13.18%) ⬆️
plotly_resampler/aggregation/algorithms/lttb_c.py 100.00% <100.00%> (ø)
...tly_resampler/figure_resampler/figure_resampler.py 90.35% <100.00%> (ø)
...ler/figure_resampler/figure_resampler_interface.py 99.73% <100.00%> (ø)
...sampler/figure_resampler/figurewidget_resampler.py 99.03% <100.00%> (ø)
plotly_resampler/figure_resampler/utils.py 96.29% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jvdd
Copy link
Member

jvdd commented Dec 27, 2022

I think merging this PR will make you (@jayceslesar) the first contributor besides @jonasvdd & me to make improvements to the Python code 🎉 🥂

And ofc happy holidays!! 🎅 🎆

@jonasvdd
Copy link
Member

Thanks a lot @jayceslesar! I'll merge this one!
🤝

@jonasvdd jonasvdd merged commit a01f0bc into predict-idlab:main Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants