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

Bug fix from get_files method. #58

Merged
merged 3 commits into from
Feb 19, 2025
Merged

Conversation

chickenchickenlove
Copy link
Contributor

Background

content_service.get_files() has potential bugs. because it uses httpx.client().

I assume the following scenario.
I send a request to Central Dogma with pattern /*s.json.
There are three possible cases

  • Case1. If multiple files are found, httpx.client.json() returns a list[dict[str, str]].
  • Case2. If only one file is found, httpx.client.json() returns a dict[str,str]
  • Case3. No contents

In case2, the Central Dogma client throws an unexpected error.
Because Content.from_dict(content) receives an unexpected argument of type str.

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2025

CLA assistant check
All committers have signed the CLA.

@chickenchickenlove
Copy link
Contributor Author

Hi, @hexoul, @ikhoon .
I made a PR to fix tiny bug.
When you have time, could you take a look?
Thanks a lot 🙇‍♂️

@hexoul
Copy link
Collaborator

hexoul commented Jan 31, 2025

Thanks for your interest and reporting!

@ikhoon ikhoon added the defect label Feb 3, 2025
@ikhoon ikhoon added this to the 0.5.0 milestone Feb 3, 2025
Copy link
Collaborator

@hexoul hexoul 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 providing reproducible scenario by adding integration tests. I think your first commit to fix the issue is a better approach than throwing an exception.

@chickenchickenlove
Copy link
Contributor Author

chickenchickenlove commented Feb 12, 2025

@hexoul Thanks for your review 🙇‍♂️
I made a new commit.
In my local, black didn't complain at all.
But, black in CI complain... 😓

# In my local
$ black --check .
All done! ✨ 🍰 ✨
37 files would be left unchanged.

What should I do?
Thanks in advance!

@hexoul
Copy link
Collaborator

hexoul commented Feb 14, 2025

In my local, black didn't complain at all.

@chickenchickenlove One possibility is that there is a difference in the version of black.
Please check if your local environment uses venv and installed packages are from pyproject.toml and uv.lock.

(py3.13) $ black --version
black, 24.10.0 (compiled: yes)
Python (CPython) 3.13.0

Copy link
Collaborator

@hexoul hexoul left a comment

Choose a reason for hiding this comment

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

LGTM. Please fix black issue.

@chickenchickenlove
Copy link
Contributor Author

@hexoul Thanks a lot.
After fixing thing which you point out, lint error has been fixed!
When you have time, please take another look!

@hexoul hexoul merged commit 80c76e8 into line:main Feb 19, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants