Skip to content

Add BarSpecification to msgspec encoding hooks #2373

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

Conversation

pierianeagle
Copy link
Contributor

@pierianeagle pierianeagle commented Feb 27, 2025

Pull Request

This PR adds support for issue #2371

Type of change

  • New feature (non-breaking change which adds functionality)

How has this change been tested?

Tested manually in a backtest.

@CLAassistant
Copy link

CLAassistant commented Feb 27, 2025

CLA assistant check
All committers have signed the CLA.

@cjdsellers cjdsellers changed the title Add BarSpecification to msgspec_encoding_hook and msgspec_decoding_hook Add BarSpecification to msgspec encoding hooks Feb 27, 2025
@cjdsellers
Copy link
Member

cjdsellers commented Feb 27, 2025

Hi @pierianeagle

Thanks for the contribution! I think this change is reasonable.

Looks like the pre-commit step in CI failed on the ruff static checks though:

ruff.....................................................................Failed
- hook id: ruff
- exit code: 1
- files were modified by this hook

nautilus_trader/common/config.py:82:5: C901 `msgspec_encoding_hook` is too complex (11 > 10)
   |
82 | def msgspec_encoding_hook(obj: Any) -> Any:
   |     ^^^^^^^^^^^^^^^^^^^^^ C901
83 |     if isinstance(obj, Decimal):
84 |         return str(obj)
   |

nautilus_trader/common/config.py:108:5: C901 `msgspec_decoding_hook` is too complex (11 > 10)
    |
108 | def msgspec_decoding_hook(obj_type: type, obj: Any) -> Any:
    |     ^^^^^^^^^^^^^^^^^^^^^ C901
109 |     if obj_type in (Decimal, pd.Timestamp, pd.Timedelta):
110 |         return obj_type(obj)
    |

You can check on your end by running pre-commit:

make pre-commit

or

pre-commit run --all-files

For this one I'd suggest just adding a noqa: C901 (too complex) beside the function definitions on L82 and L108.
These functions probably need to use match statements instead, but thats something we can do after this merges.

Also the pre-commit will probably make some additional formatting changes, so please run and commit any other changes as well then push again 👍

@pierianeagle
Copy link
Contributor Author

Cheers, I've sorted that out and pushed the lastest changes

@cjdsellers
Copy link
Member

Great, thanks for adding the tests also 🙏

@cjdsellers cjdsellers merged commit 8333898 into nautechsystems:develop Feb 28, 2025
12 checks passed
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.

3 participants