Skip to content

Bug fix from get_files method.#58

Merged
hexoul merged 3 commits intoline:mainfrom
chickenchickenlove:250124-bug-fix
Feb 19, 2025
Merged

Bug fix from get_files method.#58
hexoul merged 3 commits intoline:mainfrom
chickenchickenlove:250124-bug-fix

Conversation

@chickenchickenlove
Copy link
Contributor

@chickenchickenlove chickenchickenlove commented Jan 24, 2025

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 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@chickenchickenlove
Copy link
Contributor Author

chickenchickenlove commented Jan 24, 2025

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

hexoul
hexoul reviewed Jan 31, 2025
View reviewed changes
@hexoul
Copy link
Collaborator

hexoul commented Jan 31, 2025 •
edited
Loading

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
hexoul
hexoul reviewed Feb 12, 2025
View reviewed changes
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 •
edited
Loading

@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

hexoul
hexoul approved these changes Feb 14, 2025
View reviewed changes
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

chickenchickenlove commented Feb 14, 2025

@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

Reviewers

@hexoul hexoul hexoul approved these changes

@ikhoon ikhoon Awaiting requested review from ikhoon

Assignees

No one assigned

Labels

defect

Projects

None yet

Milestone

0.5.0

Development

Successfully merging this pull request may close these issues.

4 participants

@chickenchickenlove @CLAassistant @hexoul @ikhoon