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

New functionality for UK Sold house price data #3969

Merged
merged 44 commits into from
Feb 2, 2023

Conversation

kulbinderdio
Copy link
Contributor

Description

  • [ New functionality under Alternative to retrieve UK sold house price data] Summary of the change / bug fix.
  • Link # issue, if applicable.
  • [

image

] Screenshot of the feature or the bug before/after fix, if applicable. - [ ] Relevant motivation and context. - [ SPARQLWrapper] List any dependencies that are required for this change.

How has this been tested?

  • Please describe the tests that you ran to verify your changes.
    Have ran all commands from application and also created a test class
  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.
  • Make sure affected commands still run in terminal
  • Ensure the SDK still works
  • Check any related reports

Checklist:

Others

  • [ x] I have performed a self-review of my own code.
  • [ x] I have commented my code, particularly in hard-to-understand areas.

@reviewpad reviewpad bot added the feat M Medium T-Shirt size feature label Jan 14, 2023
@luqmanbello luqmanbello changed the base branch from develop to main January 16, 2023 14:35
@luqmanbello luqmanbello changed the base branch from main to develop January 16, 2023 14:35
@reviewpad reviewpad bot added feat XL Extra Large feature feat M Medium T-Shirt size feature and removed feat M Medium T-Shirt size feature feat XL Extra Large feature labels Jan 16, 2023
@Chavithra Chavithra self-requested a review January 17, 2023 16:54
@kulbinderdio
Copy link
Contributor Author

Hi, is there something else required for me to do now?

@jmaslek
Copy link
Collaborator

jmaslek commented Jan 26, 2023

Sorry for the long delay. Will go through now.

First thing I see is that in the menu:

│                                                                                                                                      │
│     sales              list UK house sales via postcode                                                                              │
│                                                                                                                                      │
│ alternative/realestate/_postcode:                                                                                                    │
│                                                                                                                                      │
│     townsales          list UK house sales via town                                                                                  │
│                                                                                                                                      │
│ alternative/realestate/_town:                                                                                                        │
│ alternative/realestate/_startdate <YYYY-MM-DD>:                                                                                      │
│ alternative/realestate/_enddate <YYYY-MM-DD>:                                                                                        │
│                                                                                                                                      │
│     regionstats        retrieve UK sold house price stats by region                                                                  │
│                                                                                                                                      │
│ alternative/realestate/_region:                                                                                                      │
│ alternative/realestate/_startdate <YYYY-MM-DD>:                                                                                      │
│ alternative/realestate/_enddate <YYYY-MM-DD>:        

Looks like you are missing the entries for the _town/_start etc in the i18 file.

Comment on lines 106 to 107
else:
console.print("[red]Select valid postcode[/red]\n")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't need to be here. This triggers the select valid postcode.

@jmaslek
Copy link
Collaborator

jmaslek commented Jan 26, 2023

If i look for help :

2023 Jan 25, 20:27 (🦋) /alternative/realestate/ $ sales -h

usage: sales [-p postcode] [-h] [--export EXPORT] [--sheet-name SHEET_NAME [SHEET_NAME ...]]

Select the postcode you want to see sold house price data for. [Source: UK Land Registry]

optional arguments:
  -p postcode, --postcode postcode
                        Postcode (default: None)
  -h, --help            show this help message (default: False)
  --export EXPORT       Export raw data into csv, json, xlsx (default: )
  --sheet-name SHEET_NAME [SHEET_NAME ...]
                        Name of excel sheet to save data to. Only valid for .xlsx files. (default: None)

For more information and examples, use 'about sales' to access the related guide.

Select valid postcode

The select valid postcode shouldnt appear if we have the -h flag. I pointed out where this happens in the code

@jmaslek
Copy link
Collaborator

jmaslek commented Jan 26, 2023

If i look for help :

2023 Jan 25, 20:27 (🦋) /alternative/realestate/ $ sales -h

usage: sales [-p postcode] [-h] [--export EXPORT] [--sheet-name SHEET_NAME [SHEET_NAME ...]]

Select the postcode you want to see sold house price data for. [Source: UK Land Registry]

optional arguments:
  -p postcode, --postcode postcode
                        Postcode (default: None)
  -h, --help            show this help message (default: False)
  --export EXPORT       Export raw data into csv, json, xlsx (default: )
  --sheet-name SHEET_NAME [SHEET_NAME ...]
                        Name of excel sheet to save data to. Only valid for .xlsx files. (default: None)

For more information and examples, use 'about sales' to access the related guide.

Select valid postcode

The select valid postcode shouldnt appear if we have the -h flag. I pointed out where this happens in the code

If i look for help :

2023 Jan 25, 20:27 (🦋) /alternative/realestate/ $ sales -h

usage: sales [-p postcode] [-h] [--export EXPORT] [--sheet-name SHEET_NAME [SHEET_NAME ...]]

Select the postcode you want to see sold house price data for. [Source: UK Land Registry]

optional arguments:
  -p postcode, --postcode postcode
                        Postcode (default: None)
  -h, --help            show this help message (default: False)
  --export EXPORT       Export raw data into csv, json, xlsx (default: )
  --sheet-name SHEET_NAME [SHEET_NAME ...]
                        Name of excel sheet to save data to. Only valid for .xlsx files. (default: None)

For more information and examples, use 'about sales' to access the related guide.

Select valid postcode

The select valid postcode shouldnt appear if we have the -h flag. I pointed out where this happens in the code

This happens for all other functions.

@jmaslek
Copy link
Collaborator

jmaslek commented Jan 26, 2023

These functions should have a limit in the views. For example, I run townsales -t London -s 2022-01-01 -e 2022-01-10 and it populated my whole IDE.

additionally, we probably want to specify a default start and end date (if I do townsales -t London it should work, but it currently does not)

@kulbinderdio
Copy link
Contributor Author

default limit values added to model functions.

@kulbinderdio
Copy link
Contributor Author

I just applied a branch update and now I'm getting Linting errors in my controller that is a new file but was formatted ok prior to the branch update. Since the file is new and only in my pull request don't understand why it is failing now

@jmaslek
Copy link
Collaborator

jmaslek commented Feb 1, 2023

I just applied a branch update and now I'm getting Linting errors in my controller that is a new file but was formatted ok prior to the branch update. Since the file is new and only in my pull request don't understand why it is failing now

The black formatter published a new version last night, which is what gets installed on the linter. You can locally run pip install -U black then reformat to fix

@kulbinderdio
Copy link
Contributor Author

Reviewpad failed with the error API rate limit exceeded for installation ID 30911239.

@jmaslek
Copy link
Collaborator

jmaslek commented Feb 2, 2023

Sorry for the delay. Was busy getting that last release fixed. If you fix the merge conflicts, I will give a final look today!

@kulbinderdio
Copy link
Contributor Author

Have resolved merge issues but now failing on some security checks I haven't seen before to do with Snyk.

@jmaslek
Copy link
Collaborator

jmaslek commented Feb 2, 2023

Something is still not correct with the poetry lock.The unit test check is failing on install dependencies.

poetry export -f requirements.txt -E optimization --without-hashes --output requirements.txt
poetry export -f requirements.txt --dev -E all --without-hashes --output requirements-full.txt

@kulbinderdio
Copy link
Contributor Author

I ran the poetry export commands and can't see any install errors but can see that some of not tests were skipped. Not sure how to resolve.

@jmaslek
Copy link
Collaborator

jmaslek commented Feb 2, 2023

I ran the poetry export commands and can't see any install errors but can see that some of not tests were skipped. Not sure how to resolve.

ill take a stab at

@jmaslek
Copy link
Collaborator

jmaslek commented Feb 2, 2023

I ran the poetry export commands and can't see any install errors but can see that some of not tests were skipped. Not sure how to resolve.

Everything was good on the update. Not sure what the snyk issues are, but I can ignore them for now. I just added a test script for our end and the rest looks good!

Copy link
Collaborator

@jmaslek jmaslek left a comment

Choose a reason for hiding this comment

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

Thanks for your patience!

@kulbinderdio
Copy link
Contributor Author

No problem. I know you're busy.
Thanks a lot for your help. Been over a decade since I last worked on a large multi developer project.

@jmaslek jmaslek merged commit 3664291 into OpenBB-finance:develop Feb 2, 2023
@kulbinderdio kulbinderdio deleted the feature/real_estate branch February 9, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat M Medium T-Shirt size feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants