thinh-vu / vnstock

A powerful Python library for getting rich data from the Vietnam Stock Market using just a few lines of code
https://vnstocks.com
Other
499 stars 130 forks source link

Test: update test case #105

Closed andrey-jef closed 8 months ago

andrey-jef commented 8 months ago

Took 17s to verify whether the output of each function (with default param input) in funds.py, is a dataframe.

Before writing more test cases, can u take a look at this discussion #103 ? I have an interest to refactor the code in funds.py by removing the logic of renaming column label. Pls share ur thoughts in that thread.

thinh-vu commented 8 months ago

Took 17s to verify whether the output of each function (with default param input) in funds.py, is a dataframe.

Before writing more test cases, can u take a look at this discussion #103 ? I have an interest to refactor the code in funds.py by removing the logic of renaming column label. Pls share ur thoughts in that thread.

Mình có sửa lại vài dòng trong code của bạn:

Mình merge code này vào nhánh beta. Bạn có thể viết tiếp nha

andrey-jef commented 8 months ago

Hi welcome back

1/ B thêm đoạn if main để chặn main script ko chạy khi import vào file khác, để làm gì vậy nhỉ?

M đang hiểu file này chỉ chạy duy nhất khi pytest ./tests/test_funds.py

Nên việc thêm phần if main là thừa và dẫn đến import unittest là ko cần thiết.

Phần if main là good practice nếu b có viết lệnh chạy trong file và muốn ngăn ko cho các lệnh này được thực thi khi import file này vào các module khác.

Mong được nghe kiến giải của b

2/ m đã có commit cái test thứ 2 để loại bỏ class (mà b ko chọn merge). Vì với mục đích hiện tại chỉ test xem các hàm có trả về được 1 df hay ko. Thì việc chia class để phân loại nhóm test case như m đã làm ở lần commit đầu là ko hữu ích

3/ m vẫn chờ b trả lời xem có đồng ý với việc m đề xuất

B quyết định ủng hộ hay bác bỏ cái đề xuất này thì mới viết tiếp test case được

thinh-vu commented 8 months ago

Hi @andrey-jef 1/ Mình không rõ bạn sử dụng module test này như nào vì không có document cụ thể và mình cũng chưa bao giờ dùng module này do vậy mình bổ sung thêm 1 vài dòng code để có thể chạy nguyên file python này từ terminal. image

2/ Chắc có mấy commit mình không theo dõi hết nên merge bị thiếu thôi. Hiện tại với số lượng hàm và module không quá nhiều thì với mình làm thêm bộ test chưa thực sự quan trọng vì mất công viết thêm module trong khi làm demo file và document mình cũng test hết lượt. Anw, biết cũng tốt vì mai mốt sẽ có lúc cần nên mình open để bạn suggest thêm và mình cũng học hỏi thêm.

3/Theo mình thì bạn vẫn để tùy chọn language được, mặc định là tiếng Anh nguyên bản như tên cột trả về từ API. nếu ai thích dùng tiếng Việt thì chọn lang='vi'. Tên cột thì bạn chuẩn hóa theo snake_case được rồi.

andrey-jef commented 8 months ago

1/ Oh. Là m sơ suất. M thấy b ko trao đổi thảo luận gì thêm nên cứ đinh ninh là b đã dùng qua pytest rồi. M sẽ bổ sung readme.md vào thư mục tests

Đây là ví dụ output ra terminal khi dùng pytest với test case cũ (chia nhóm theo class) và ko có phần if main.

vnstock on  main [!?⇡] is 󰏗 v0.2.8.7.dev.22  via  v3.9.18 via  dev-vnstock
❯ pytest .\tests\
====================================================================== test session starts =======================================================================
platform win32 -- Python 3.9.18, pytest-7.4.3, pluggy-1.3.0
rootdir: ~\my_workspace\andrey-jef\vnstock
plugins: cov-4.1.0
collected 4 items

tests\test_funds.py ....                                                                                                                                    [100%]

======================================================================= 4 passed in 8.38s ======================================================================== 

2/ B quen với cách test nào thì cứ dùng tiếp thôi nhé.

Mục đích m muốn tích hợp unit test là để bản thân m khi chỉnh sửa funds.py. M sẽ chỉ chạy pytest trước khi commit. Thay vì phải chạy lại test_notebook.ipynb và quan sát kết quả. Nên yêu cầu tích hợp cái test này là phục vụ cho sự tiện lợi của bản thân m khi đóng góp mã nguồn. Chứ ko phải vì m muốn đề xuất thay đổi cách b làm việc.

3/ Nếu vậy khi chọn lang=vi thì tên cột vẫn để theo Title Case và ký tự tiếng Việt hả b? Chỉ khi lang=en thì tên cột mới chuyển thành snake_case và là ký tự tiếng Anh?

Cá nhân m đang thử viết extension để đổ thẳng dữ liệu từ query của vnstock vào openbb mà ko cần manual load via csv. Việc b cho phép nhiều biến thể column label dựa trên lựa chọn lang khiến m khá bối rối khi phải chọn column label để đổ vào pydantic data model theo hướng dẫn của openbb

vd như với model này, m nên chọn alias của tên cột xuất ra từ vnstock là tiếng Anh snake_case, hay tiếng Việt Title Case, và từ phía openbb nên kỳ vọng default input param cho query của vnstock là gì, output từ df khi gọi query qua vnstock sẽ có tên cột ntn? Đó là những vấn đề m đang nghĩ tới khi vnstock ko thống nhất được quy chuẩn về tên cột và docs cũng chưa đề cập rõ về data model (vì lý do vnstock cần phát triển feature hơn là chú ý vào mô hình dữ liệu nền tảng mà b đã nói qua và m cũng ko có gì phản bác)

class FundInfo(Data):
    """Fmarket Fund info data model."""

    __alias_dict__ = {
        'fund_id': 'id',
        'short_name': 'shortName',
        'name': 'name',
        'inception_date': 'firstIssueAt',
        'management_fee': 'managementFee',
        'nav': 'nav',
        'fund_owner': 'owner',
        'asset_type': 'dataFundAssetType',
        'nav_change': 'productNavChange',
    }

    fund_id: int
    short_name: str
    name: str
    inception_date: datetime | None = None
    management_fee: float | None = None
    nav: float | None = None
    fund_owner: FundOwner | None = None
    asset_type: FundAssetTypes | None = None
    nav_change: FundNavChange | None = None
thinh-vu commented 8 months ago

Cám ơn @andrey-jef là chia sẻ rõ hơn về chủ đề thảo luận này. Mình phản hồi như sau: 1 + 2 mình okay nhé.

  1. Sau khi suy nghĩ mình định hướng sẽ sử dụng snake_case cho dữ liệu trả về từ vnstock và trong thời gian tới mình sẽ chuyển đổi dần cách trả dữ liệu từ các hàm hiện tại cũng như hàm sẽ bổ sung thêm theo định dạng này. Lý do: a) Dễ đọc, dễ sử dụng chung cho nhiều use case khác nhau bao gồm lưu trữ vào cơ sở dữ liệu b) hạn chế lẫn lộn giữa các biến thể camelCase hay TitleCase. Mình chưa nghiên cứu kỹ dữ liệu đầu vào của OpenBB nên nếu bạn có thông tin nghiên cứu rồi thì chia sẻ luôn giúp mình. Anw, thống nhất sử dụng snake_case thì bạn có thể mapping tên cột hiện tại với tiêu chuẩn này để làm tiếp thuận lợi hơn.