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

[1.0] Remove deprecations #460

Merged
merged 4 commits into from
Oct 18, 2021
Merged

[1.0] Remove deprecations #460

merged 4 commits into from
Oct 18, 2021

Conversation

WardBrian
Copy link
Member

Closes #430. This should probably be the last thing we merge before cutting a release, hence the draft status.

Submission Checklist

  • Run unit tests
  • Declare copyright holder and open-source license: see below

Summary

  • Removed all deprecated features

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Simons Foundation

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@WardBrian
Copy link
Member Author

Seems like Pylint has updated again, ugh.

@mitzimorris
Copy link
Member

this one: Formatting a regular string which could be a f-string (consider-using-f-string)
keep or ignore?

@WardBrian
Copy link
Member Author

It's good advice in most cases, except for logging (f-strings are greedy evaluated, as opposed to %-formatting). I will check if these warnings include logging statements - if so, we should probably disable it

@WardBrian
Copy link
Member Author

Looking through, they don't include logging statements, so I'd consider them all legit. I can open a separate PR which fixes, it's a lot of minor one-line rewrites

@mitzimorris
Copy link
Member

mitzimorris commented Sep 23, 2021

so disable for now, address in separate PR? sounds good to me.
there are close to 300 of these. ugh.

@WardBrian
Copy link
Member Author

WardBrian commented Sep 24, 2021

Yeah, this is actually a huge pain. f-strings are great (easier to read imo, and also faster by a not-insignificant margin) but they do not play nicely with a short line length limit and long variable names. I'm just going to disable it for now, maybe we fix it one day maybe we don't. Tools like flynt can help with the conversion, but they won't catch all cases and won't do the manual line-splitting we need to satisfy flake8 at 80 characters

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2021

Codecov Report

Merging #460 (78a8e9d) into develop (f048803) will decrease coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #460      +/-   ##
===========================================
- Coverage    78.09%   77.71%   -0.39%     
===========================================
  Files           30       20      -10     
  Lines         9255     5976    -3279     
===========================================
- Hits          7228     4644    -2584     
+ Misses        2027     1332     -695     
Impacted Files Coverage Δ
cmdstanpy/cmdstanpy/model.py 85.75% <0.00%> (-0.89%) ⬇️
...runner/work/cmdstanpy/cmdstanpy/cmdstanpy/model.py 85.75% <0.00%> (-0.89%) ⬇️
cmdstanpy/cmdstanpy/__init__.py 86.95% <0.00%> (-0.55%) ⬇️
...ner/work/cmdstanpy/cmdstanpy/cmdstanpy/__init__.py 86.95% <0.00%> (-0.55%) ⬇️
a/cmdstanpy/cmdstanpy/cmdstanpy/_version.py
a/cmdstanpy/cmdstanpy/cmdstanpy/__init__.py
a/cmdstanpy/cmdstanpy/cmdstanpy/cmdstan_args.py
a/cmdstanpy/cmdstanpy/cmdstanpy/compiler_opts.py
a/cmdstanpy/cmdstanpy/cmdstanpy/model.py
...tanpy/cmdstanpy/cmdstanpy/install_cxx_toolchain.py
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f048803...78a8e9d. Read the comment docs.

@WardBrian WardBrian marked this pull request as ready for review October 12, 2021 15:14
Copy link
Member

@mitzimorris mitzimorris left a comment

Choose a reason for hiding this comment

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

LGTM!

@WardBrian WardBrian mentioned this pull request Oct 15, 2021
@WardBrian
Copy link
Member Author

Merging in anticipation of a 1.0 (possibly a RC first)

@WardBrian WardBrian merged commit 63b46d4 into develop Oct 18, 2021
@WardBrian WardBrian deleted the remove-deprecations branch October 18, 2021 13:39
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.

[1.0 Release] List of expiring deprecations
3 participants