Refactor: improve code quality and clean up technical debt #345
Conversation
- Remove `@runtime_checkable` as unnecessary and incomplete (see note in https://docs.python.org/3/library/typing.html#typing.runtime_checkable) - Add new protocols - Add `@abstractmethod` to ensure the functions should always be implemented before instantiation of class
- Remove `/jira/utils.py` and `/utils/dates.py`
- Add `/utils/date.py`
- Only preserve `parse_date(date_str: str | None) -> datetime | None`
- It is unnecessary for MCP Server to parse dates INTO different
formats (e.g. human readable, ymd), since the format is unimportant
to LLM.
- Use `dateutil.parser.parse` as default: It provides much
flexibility. We do not need to handle different formats of date by
ourselves.
- Change the return type from `str` to `datetime | None`: It would be
better if models can use `datetime` to storage informations, as it
is formatted explicitly. If we use `str`, we will have to guess the
format of the date, or always call `parse_date` to ensure
flexibility.
- Remove other functions
- `get_mixin_method`: It is not suggested to implement the same
method in multiple mixins.
- `escape_jql_string`: Unused.
- Rename `JiraClient._field_ids` to `_field_ids_cache` to ensure name consistency - Move the type hint to top of the class to make it clearer and better type hints
- Change `JiraClient._field_ids_cache` from `dict[str, str] | None` to
`list[dict[str, Any]] | None`
- Ensure consistency with API.
- Clean `get_jira_field_ids`
- Remove it in `epics.py` and `fields.py`
- Add it in `fields.py` using implementation in `epics.py`
- `JiraFetcher` inherits `EpicsMixin` first so it is using the
implementation from `EpicsMixin`.
- Rename it to `FieldsMixin.get_field_ids_to_epic`
- Update protocols
- Remove `_parse_date` in `search.py`
- Remove `hasattr` check in `FieldsMixin.format_field_value`
- `JiraFetcher` always inherits from `UsersMixin` so it is impossible
for it to not have `_get_account_id`.
- In this case, it is impossible to return `{"name": value}`.
FIXME: `format_field_value` needs to be fixed for server/DC cases
- Remove `_parse_date` and use `utils.parse_date` for consistency - Add protocols - Add necessary type checks (to fix linter errors) - Remove unnecessary `hasattr`
- Move `JiraChangelog` to `/models/jira/common.py` - Change `JiraChangelog.created` from `str` to `datetime | None`
- Remove `get_jira_field_ids` (which is renamed as
`FieldsMixin.get_field_ids_to_epic`)
- It may be not reasonable for all usages in `IssuesMixin` to call
`get_field_ids_to_epic`, but in actual situation it does call
`get_field_ids_to_epic`. See also: commit
668784d
- Remove `_parse_date` and use `utils.parse_date` for consistency
- Remove unnecessary `hasattr`
- Add necessary type checks to fix linter errors
- Inherit mixins from `JiraFetcher` instead of mixins itself
- This is because the mixins are highly coupling.
- Expose it as mixin itself to type checker, to ensure there are no
external callings to other mixins.
- Add type checks and type hints
- Remove unnecessary or duplicated tests
- Remove some `delattr` usages
- It is not suggested to delete a class's attributes because if you do
that, you will have to check ALL usages when you use ANY attributes
because even IDE or linter hints you that the class has this
attribute, the attribute still would be deleted and therefore not
exists. This is fatal.
- Remove `delattr` since it is fatal - Adapt to the rename of `get_jira_field_ids` to `get_field_ids_to_epic`
|
jira_client = <mcp_atlassian.jira.JiraFetcher object at 0x000001B20BB69B80>
@pytest.mark.anyio
async def test_jira_get_issue_link_types(jira_client: JiraFetcher) -> None:
"""Test retrieving issue link types from Jira."""
# Initialize the LinksMixin
links_client = LinksMixin(config=jira_client.config)
# Get the issue link types
link_types = links_client.get_issue_link_types()
# Verify the response
assert isinstance(link_types, list)
> assert len(link_types) > 0
E assert 0 > 0
E + where 0 = len([])
tests\test_real_api_validation.py:1453: AssertionError |
cutekibry
changed the title
|
@cutekibry I apologize for the delayed response. I've been quite busy recently and haven't had the chance to review this. I'll make sure to review and merge this PR either tonight or tomorrow. Thank you so much for your substantial contributions to the project! |
Never mind, it is not your main work so do not feel too much stress doing it. Even if you do not approve these PR, I still will merge them to my nightly build, that is to say, my work won't be interrupted. I sincerely hope you are happy doing this project. Tomorrow is 5/1, wish you have a nice May Day. |
- Replace the deprecated `createmeta` method with `issue_createmeta_fieldtypes` for fetching required fields. - Implement logic to retrieve issue type ID using `get_project_issue_types`. - Enhance error handling and logging for better debugging. - Update unit tests to mock new API responses and validate functionality.
- Update the test to allow for an empty list as a valid response when no link types are configured. - Refactor assertions to check the structure of the first element only if the list is not empty. - Improve the docstring for clarity on expected behavior when issues are not found in attachment tests.
There was a problem hiding this comment.
I've made a few test fixes and pushed them directly to your branch. All CI checks are now passing.
I fixed the test_jira_get_issue_link_types to handle cases where the result is empty (count of 0), and properly implemented the get_required_fields method to restore the tests.
The Server/DC compatibility issue with FieldsMixin.format_field_value that we discussed will be tracked as a separate issue (#355) and addressed in a follow-up PR.
Now that all tests are passing and the refactoring looks great, I'm going to merge this PR. Thank you again for your valuable contribution with this important refactoring work!
Description
This PR focuses on comprehensive code quality improvements, with the primary goal of eliminating all linter errors in
/jira/*.py. The changes include date handling unification, type system enhancements, and code duplication removal.Changes
1. Date Handling Unification
utils/date.py:dateutil.parser.parsefor more flexible date parsingdatetime | Nonefor better type safety<Mixin>._parse_datemethods as they were unnecessary for LLM processing2. Type System Improvements
protocol.py:@abstractmethoddecorators to enforce implementation requirements@runtime_checkabledecorators (see Python docs).pylintrcto express the using off-strin loggers explicitly to linterhasattrchecks:3. Code Duplication Elimination
get_jira_field_idsimplementations:EpicsMixin,IssuesMixin, andFieldsMixinFieldsMixinusingEpicsMixin's logic (JiraFetcherinheritsEpicsMixinfirst)get_field_ids_to_epicfor clarityget_epic_issuesimplementations:SearchMixinEpicsMixinunchangedJiraClient._field_id:_field_ids_cachefor better naming consistencydict[str, str] | Nonetolist[dict[str, Any]] | Noneto match API structure4. Test Suite Improvements
delattrusage in tests:Critical Fixes Needed
FieldsMixin.format_field_valueserver/DC compatibility (ref: 668784d):{"name": value}for server/DC casesJiraFetcheralways inherits fromUsersMixin, making_get_account_idalways availabletest_jira_get_issue_link_typesnot passedTesting and Validation
Checklist