Add get_dividends function and fix ISSClient query parameter merging#2
Open
ncaraw04-cell wants to merge 2 commits intombk-dev:masterfrom
Open
Add get_dividends function and fix ISSClient query parameter merging#2ncaraw04-cell wants to merge 2 commits intombk-dev:masterfrom
ncaraw04-cell wants to merge 2 commits intombk-dev:masterfrom
Conversation
Reviewer's GuideAdds a new get_dividends helper for fetching dividend history via ISSClient, exports it from the package, and fixes ISSClient query parameter merging to avoid TypeError when overlapping parameters like iss.meta are used, along with a basic regression test. Sequence diagram for get_dividends using ISSClient with fixed query mergingsequenceDiagram
actor Caller
participant RequestsModule
participant ISSClient
participant ISSClient_make_query as ISSClient_make_query
participant HTTPClient
Caller->>RequestsModule: get_dividends(session, security, kwargs)
RequestsModule->>RequestsModule: build url for security
RequestsModule->>RequestsModule: query = {iss.meta: off}
RequestsModule->>RequestsModule: query.update(kwargs)
RequestsModule->>ISSClient: create ISSClient(session, url, query)
RequestsModule->>ISSClient: get(start)
ISSClient->>ISSClient_make_query: _make_query(start)
ISSClient_make_query->>ISSClient_make_query: query = BASE_QUERY.copy()
ISSClient_make_query->>ISSClient_make_query: query.update(self._query)
ISSClient_make_query-->>ISSClient: merged query
ISSClient->>HTTPClient: HTTP GET url with merged query
HTTPClient-->>ISSClient: JSON response
ISSClient-->>RequestsModule: res dict
RequestsModule->>RequestsModule: dividends = res.get(dividends, [])
RequestsModule-->>Caller: dividends list
Class diagram for ISSClient and new get_dividends helperclassDiagram
class ISSClient {
- session : requests.Session
- url : str
- query : Dict~str, Any~
+ get(start : int) Dict~str, Any~
- _make_query(start : int) Dict~str, Union~str, int~
}
class RequestsModule {
+ get_dividends(session : requests.Session, security : str, kwargs : Any) List~Dict~str, Any~~
}
class BASE_QUERY {
<<dict>>
}
RequestsModule ..> ISSClient : creates
ISSClient ..> BASE_QUERY : uses in _make_query
%% get_dividends logic
%% RequestsModule : query = {iss.meta: off}
%% RequestsModule : query.update(kwargs)
%% RequestsModule : iss = ISSClient(session, url, query)
%% RequestsModule : res = iss.get()
%% RequestsModule : return res.get(dividends, [])
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- For consistency with the other request helpers, consider reusing existing internal helpers (like
_get_short_data) inget_dividendsinstead of manually constructing and callingISSClientdirectly. - In
get_dividends, the explicitquery = {"iss.meta": "off"}followed byquery.update(kwargs)allows callers to overrideiss.meta; if the intent is to always suppress metadata, you may want to prevent kwargs from overriding that key. - The explicit
__all__list inmoex/__init__.pynow duplicates the public surface ofrequests; this will require manual updates for every new helper, so consider deriving this fromrequestsor limiting__all__maintenance to a single place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For consistency with the other request helpers, consider reusing existing internal helpers (like `_get_short_data`) in `get_dividends` instead of manually constructing and calling `ISSClient` directly.
- In `get_dividends`, the explicit `query = {"iss.meta": "off"}` followed by `query.update(kwargs)` allows callers to override `iss.meta`; if the intent is to always suppress metadata, you may want to prevent kwargs from overriding that key.
- The explicit `__all__` list in `moex/__init__.py` now duplicates the public surface of `requests`; this will require manual updates for every new helper, so consider deriving this from `requests` or limiting `__all__` maintenance to a single place.
## Individual Comments
### Comment 1
<location path="tests/test_requests.py" line_range="297-302" />
<code_context>
assert data[25]['till'] == '2023-03-03'
- assert data[35]['tradingsession'] == 3
+
+def test_get_dividends(session):
+ data = requests.get_dividends(session, "SBER")
+ assert isinstance(data, list)
+ assert len(data) > 0
+ assert data[0]["secid"] == "SBER"
+ assert "value" in data[0]
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `test_get_dividends` to cover edge cases such as empty results and custom query parameters.
The current test only covers the happy path for a known security. Please also add cases for:
- A security with no dividend history (should return an empty list without raising).
- Passing extra query parameters via `**kwargs` (e.g., date filters) to ensure they’re forwarded and handled.
- Overriding `iss.meta` via `**kwargs` to confirm user-supplied values are respected.
These will better exercise the query-building logic and robustness of `get_dividends`.
Suggested implementation:
```python
def test_get_dividends(session):
# Happy path for a known security
data = requests.get_dividends(session, "SBER")
assert isinstance(data, list)
assert len(data) > 0
assert data[0]["secid"] == "SBER"
assert "value" in data[0]
# Edge case: a date range with no expected dividends should return an empty list
empty_data = requests.get_dividends(
session,
"SBER",
from_="2100-01-01",
till="2100-12-31",
)
assert isinstance(empty_data, list)
assert len(empty_data) == 0
# Edge case: extra query parameters via **kwargs (e.g., limit)
limited_data = requests.get_dividends(session, "SBER", limit=1)
assert isinstance(limited_data, list)
assert 0 <= len(limited_data) <= 1
# Edge case: overriding iss.meta via **kwargs
no_meta_data = requests.get_dividends(session, "SBER", iss_meta="off")
assert isinstance(no_meta_data, list)
assert len(no_meta_data) > 0
assert no_meta_data[0]["secid"] == "SBER"
```
If `get_dividends` does not currently support `from_`, `till`, `limit`, or `iss_meta` as keyword arguments, you will need to:
1. Update its signature to accept `**kwargs`.
2. Ensure those `**kwargs` are forwarded into the underlying request/query-building logic (e.g., included in the params dict when calling the ISS API).
3. If the API expects `from` instead of `from_`, map `from_` to `"from"` inside `get_dividends`.
Adjust the parameter names in the test to match the actual supported query parameters in your codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/test_requests.py
Outdated
Comment on lines
+297
to
+302
| def test_get_dividends(session): | ||
| data = requests.get_dividends(session, "SBER") | ||
| assert isinstance(data, list) | ||
| assert len(data) > 0 | ||
| assert data[0]["secid"] == "SBER" | ||
| assert "value" in data[0] |
There was a problem hiding this comment.
suggestion (testing): Extend test_get_dividends to cover edge cases such as empty results and custom query parameters.
The current test only covers the happy path for a known security. Please also add cases for:
- A security with no dividend history (should return an empty list without raising).
- Passing extra query parameters via
**kwargs(e.g., date filters) to ensure they’re forwarded and handled. - Overriding
iss.metavia**kwargsto confirm user-supplied values are respected.
These will better exercise the query-building logic and robustness of get_dividends.
Suggested implementation:
def test_get_dividends(session):
# Happy path for a known security
data = requests.get_dividends(session, "SBER")
assert isinstance(data, list)
assert len(data) > 0
assert data[0]["secid"] == "SBER"
assert "value" in data[0]
# Edge case: a date range with no expected dividends should return an empty list
empty_data = requests.get_dividends(
session,
"SBER",
from_="2100-01-01",
till="2100-12-31",
)
assert isinstance(empty_data, list)
assert len(empty_data) == 0
# Edge case: extra query parameters via **kwargs (e.g., limit)
limited_data = requests.get_dividends(session, "SBER", limit=1)
assert isinstance(limited_data, list)
assert 0 <= len(limited_data) <= 1
# Edge case: overriding iss.meta via **kwargs
no_meta_data = requests.get_dividends(session, "SBER", iss_meta="off")
assert isinstance(no_meta_data, list)
assert len(no_meta_data) > 0
assert no_meta_data[0]["secid"] == "SBER"If get_dividends does not currently support from_, till, limit, or iss_meta as keyword arguments, you will need to:
- Update its signature to accept
**kwargs. - Ensure those
**kwargsare forwarded into the underlying request/query-building logic (e.g., included in the params dict when calling the ISS API). - If the API expects
frominstead offrom_, mapfrom_to"from"insideget_dividends.
Adjust the parameter names in the test to match the actual supported query parameters in your codebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ISSClient._make_querythat caused aTypeErrorwhen overlapping query parameters were used (likeiss.meta).Summary by Sourcery
Add a new helper to fetch MOEX dividend history and expose it in the public API while fixing query parameter merging in the ISS client.
New Features:
Bug Fixes:
Enhancements:
Tests: