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

Deprecate kadi load_ska_dir() in favor of parse_cm load_dir_from_load_name #310

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jan 10, 2024

Description

The kadi.commands.core function load_ska_dir() has been superceded by parse_cm.paths.load_dir_from_load_name(). This PR re-implements the kadi load_ska_dir() function as a call to load_dir_from_load_name() and issues a deprecation FutureWarning.

Interface impacts

This function is now deprecated and using it will cause unit tests to fail or issue warnings to the console.
From https://github.com/search?q=org%3Asot%20ska_load_dir&type=code it seems that only mica was using this (fixed in sot/mica#288).

Testing

Unit tests

(razl) ➜  kadi git:(deprecate-ska-load-dir) git rev-parse HEAD  
1d6011d8e0cd5ba9a2babc8dc57b1755d5e465ff
(razl) ➜  kadi git:(deprecate-ska-load-dir) pytest            
============================================================================= test session starts ==============================================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 230 items                                                                                                                                                            

kadi/commands/tests/test_commands.py ......................................................................................                                              [ 37%]
kadi/commands/tests/test_states.py ......................x.............................................x.......................                                          [ 77%]
kadi/commands/tests/test_validate.py ....................                                                                                                                [ 86%]
kadi/tests/test_events.py ..........                                                                                                                                     [ 90%]
kadi/tests/test_occweb.py ......................                                                                                                                         [100%]

================================================================== 228 passed, 2 xfailed in 98.30s (0:01:38) ===================================================================

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

No functional testing.

@jeanconn
Copy link
Contributor

I think you've used FutureWarning in recent work (like https://github.com/sot/chandra_cmd_states/blob/efd4c947c6508c63df232344d58696fa9d11a6eb/chandra_cmd_states/__init__.py#L17) so maybe that's our convention?

@taldcroft taldcroft requested a review from javierggt January 17, 2024 14:09
Copy link
Contributor

@javierggt javierggt left a comment

Choose a reason for hiding this comment

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

Looks good to me. The change is simple enough. The tests pass.

I think one can use FutureWarning instead of the numpy warning. What is the reason not to?


warnings.warn(
"ska_load_dir() is deprecated. Use parse_cm.paths.load_dir_from_load_name()",
np.VisibleDeprecationWarning,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the issue with using FutureWarning here? (as you suggest) It would seem to me that this is exactly what it is meant for, but perhaps I'm missing something.

Strictly speaking, maybe the intended use is to create our own deprecation warning class and make it inherit from FutureWarning, though I am not proposing to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I like that the numpy one has the word Deprecation in it, but I see the point and have changed this.

@taldcroft taldcroft force-pushed the deprecate-ska-load-dir branch from 2b977de to 5fcdf13 Compare January 18, 2024 20:18
@taldcroft taldcroft force-pushed the observations-from-cmds branch from 0f28614 to b5c0d6e Compare January 23, 2024 20:38
Base automatically changed from observations-from-cmds to master January 24, 2024 09:20
@taldcroft taldcroft force-pushed the deprecate-ska-load-dir branch from 3724bc9 to 1d6011d Compare January 24, 2024 09:46
@taldcroft taldcroft merged commit b5bb297 into master Jan 24, 2024
4 checks passed
@taldcroft taldcroft deleted the deprecate-ska-load-dir branch January 24, 2024 10:16
This was referenced Mar 6, 2024
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.

3 participants