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

views: issue warning on use of .daemonize #458

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

AvdN
Copy link
Contributor

@AvdN AvdN commented Jul 29, 2023

add deprecation warning
add intermediary Lona_2_0_DeprecationWarning so only one warnings needs to be set for filtering.
update example code

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (68b0445) 73.16% compared to head (e325229) 71.81%.
Report is 20 commits behind head on master.

❗ Current head e325229 differs from pull request most recent head 0ff6921. Consider uploading reports for the commit 0ff6921 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
- Coverage   73.16%   71.81%   -1.35%     
==========================================
  Files          88       84       -4     
  Lines        6103     5776     -327     
  Branches     1326     1083     -243     
==========================================
- Hits         4465     4148     -317     
+ Misses       1357     1353       -4     
+ Partials      281      275       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SmithChart
Copy link
Contributor

SmithChart commented Jul 31, 2023

No need to change this for this PR, but shouldn't we issue deprecation warning at all the other soon-to-be-deprecated features, too?

Edit: I see @fscherf has already commented on this in another PR :-) So just go ahed.

@AvdN
Copy link
Contributor Author

AvdN commented Aug 3, 2023

I'll try to update this with more warnings, but of course there is plenty of room for error, because I don't understand all of the code.

On deprecated routines I will have have warning issued in the body of the routine not in the callers place (e.g. set_client_version defined in compat.py and used in settings.py) although both have a ToDo.. for 2.0

@AvdN AvdN force-pushed the daemonize_deprecation-2 branch 2 times, most recently from 0d1a6b1 to 9e0276f Compare August 3, 2023 12:51
@fscherf
Copy link
Member

fscherf commented Aug 15, 2023

@SmithChart, @AvdN: I created #468 with a roadmap. The PR still needs review, but I think you can use the list of tasks for Lona 2 to create warnings and deprecations

Copy link
Member

@fscherf fscherf left a comment

Choose a reason for hiding this comment

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

Really nice! I like the approach. Needs only a little bit of work.

lona/compat.py Outdated
@@ -14,6 +16,7 @@
def get_client_version():
# TODO: remove in 2.0

lona.warnings.remove_2_0()
Copy link
Member

Choose a reason for hiding this comment

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

All warnings in this module are incorrect because they run no matter what the user does, and are meant to remain compatible with old API. The TODO: remove in 2.0 is only there so we don't forget to remove the functions before releasing Lona 2

@@ -123,6 +123,8 @@ def __init__(

# TODO: remove in 2.0
# compatibility for older Lona application code
if hasattr(self.view, 'STOP_DAEMON_WHEN_VIEW_FINISHES'):
Copy link
Member

Choose a reason for hiding this comment

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

This is a feature flag. Not using it should not result in a deprecation warning

@fscherf
Copy link
Member

fscherf commented Dec 3, 2023

@AvdN: This PR has been open for a long time now. Should I just take over and add the last fixes?

@AvdN
Copy link
Contributor Author

AvdN commented Dec 4, 2023

Hi @fscherf, yes please, I am not sure why I didn't react to this at the first request. Sorry for that.

@fscherf
Copy link
Member

fscherf commented Dec 6, 2023

@AvdN: There is nothing to be sorry about :)

add deprecation warning
add intermediary Lona_2_0_DeprecationWarning so only
one warnings needs to be set for filtering.
update example code
@fscherf fscherf merged commit b1b49b6 into lona-web-org:master Feb 6, 2024
5 checks passed
@fscherf
Copy link
Member

fscherf commented Feb 6, 2024

@AvdN: merged! I am so sorry that it took me so long. The test suite was broken for a long time and it took me a long time to understand why. Thanks for contributing, and for your patience!

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.

4 participants