Skip to content

Better path and string handling#106

Merged
nathan-weinberg merged 1 commit into
instructlab:mainfrom
danmcp:pathhandling
Sep 12, 2024
Merged

Better path and string handling#106
nathan-weinberg merged 1 commit into
instructlab:mainfrom
danmcp:pathhandling

Conversation

@danmcp

@danmcp danmcp commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

Removing several leftover cases of paths formed without os.path.join
Replacing string concats with f""

Comment thread src/instructlab/eval/mt_bench_judgment.py Outdated
@JamesKunstle

Copy link
Copy Markdown
Contributor

pathlib.Path is the string-handling mechanism in the CLI, would there be any interest in switching over for this library?

Removing several leftover cases of paths formed without os.path.join
Replacing string concats with f""

Signed-off-by: Dan McPherson <dmcphers@redhat.com>
@danmcp

danmcp commented Aug 26, 2024

Copy link
Copy Markdown
Contributor Author

pathlib.Path is the string-handling mechanism in the CLI, would there be any interest in switching over for this library?

I would generally prefer pathlib.Path but I was trying to make the simplest change. We also aren't really doing any operations here where the niceties of pathlib.Path would come in handy.

@nathan-weinberg

Copy link
Copy Markdown
Member

@Mergifyio refresh

@mergify

mergify Bot commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

refresh

✅ Pull request refreshed

@mergify mergify Bot added the one-approval label Aug 28, 2024
@nathan-weinberg
nathan-weinberg requested review from a team and JamesKunstle September 12, 2024 14:55
@mergify mergify Bot removed the one-approval label Sep 12, 2024
@nathan-weinberg
nathan-weinberg removed the request for review from JamesKunstle September 12, 2024 15:38
@nathan-weinberg

Copy link
Copy Markdown
Member

Mergify rules have changed since this was opened thus no automerge - doing so manually

@nathan-weinberg
nathan-weinberg merged commit 52cb38a into instructlab:main Sep 12, 2024
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.

4 participants