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

560 renaming housing recovery to housing valuation recovery #565

Merged

Conversation

longshuicy
Copy link
Member

@longshuicy longshuicy commented May 8, 2024

  1. Use the sphinx deprecation decorator
  2. Found an elegant way for he old class to call the new one
  3. Update the sphinx doc, also add the deprecation decorator
image

To test:

  1. run the old test_housingrecovery.py. This should print out warnings but still execute as it used to.
  2. run the new test_housingvaluationrecovery.py.

@navarroc
Copy link
Member

navarroc commented May 8, 2024

I think we'll need to add deprecated to the list of dependencies, right?

@longshuicy
Copy link
Member Author

I think we'll need to add deprecated to the list of dependencies, right?

Hmm sorry I don't follow. Could you give me an example?

@navarroc
Copy link
Member

navarroc commented May 8, 2024

I think we'll need to add deprecated to the list of dependencies, right?

Hmm sorry I don't follow. Could you give me an example?

I had to install the "deprecated" library into my pip environment to run the test so we might need to add it to setup.py. Looking at the conda environment for 1.18.1, it is already there (probably part of the base conda package) or another dependency we have already pulls it in.

@navarroc
Copy link
Member

navarroc commented May 8, 2024

Basic tests pass (checked old output vs new output), the old analysis shows a deprecation message. I also checked/built the module documentation, looks good.

@longshuicy
Copy link
Member Author

I think we'll need to add deprecated to the list of dependencies, right?

Hmm sorry I don't follow. Could you give me an example?

I had to install the "deprecated" library into my pip environment to run the test so we might need to add it to setup.py. Looking at the conda environment for 1.18.1, it is already there (probably part of the base conda package) or another dependency we have already pulls it in.

Ah i see! You are talking about the deprecated library. Yes I think I need to add it good catch!

@longshuicy
Copy link
Member Author

I had to install the "deprecated" library into my pip environment to run the test so we might need to add it to setup.py. Looking at the conda environment for 1.18.1, it is already there (probably part of the base conda package) or another dependency we have already pulls it in.

I added the dependencies:

  • Not sure how conda got the "deprecated" library. We are not explicitly listing it in the environment.yml... just to be safe I added there
  • For pip, the package name seems to be capitalized "Deprecated"? https://pypi.org/project/Deprecated/

@longshuicy longshuicy self-assigned this May 13, 2024
@longshuicy longshuicy requested a review from navarroc May 21, 2024 21:45
@ywkim312 ywkim312 merged commit 10404b9 into develop May 24, 2024
7 checks passed
@ywkim312 ywkim312 deleted the 560-renaming-housing-recovery-to-housing-valuation-recovery branch May 24, 2024 15:18
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.

Renaming housing recovery to housing valuation recovery
3 participants