Skip to content

Use portable collection paths in cleaner - #3111

Open
OskarEichler wants to merge 1 commit into
ruby:masterfrom
OskarEichler:codex/cleaner-portable-paths
Open

Use portable collection paths in cleaner#3111
OskarEichler wants to merge 1 commit into
ruby:masterfrom
OskarEichler:codex/cleaner-portable-paths

Conversation

@OskarEichler

Copy link
Copy Markdown
Contributor

Summary: derive collection gem and version names through Pathname components instead of splitting paths on a slash. This preserves cleaning behavior on Windows-style path implementations.

Verification: focused baseline/fixed reproduction, combined RBS 4.2.0 consumer models, and RuboCop (738 files, zero offenses). No tests are added in this PR.

Compatibility: no public API removal or dependency/version change.

@pocke pocke self-assigned this Sep 7, 2026
@pocke
pocke force-pushed the codex/cleaner-portable-paths branch from ca80360 to e422900 Compare September 7, 2026 09:41
@pocke

pocke commented Sep 7, 2026

Copy link
Copy Markdown
Member

Thank you for the patch. I would like to merge this, but as a refactoring rather than as a portability fix, so let me record why.

I tried to build the case the description points at and could not find one. Dir.glob returns /-separated paths on Windows too, and Pathname#glob assembles its results through Pathname#+, which also joins with /. Even if path: in rbs_collection.yaml were written with backslashes, only the leading component would carry them, and *_, gem_name, version = dir.to_s.split("/") takes the last two components, which are always separated by /. As far as I can tell the old and the new code produce the same gem_name and version on every platform we support.

That said, the new form reads better. dir.parent.basename and dir.basename say what they mean directly, and the two bare raises that go away could only have fired in situations the */* glob pattern already rules out. That is reason enough to take it, so I am merging it on those grounds.

I have rebased the branch onto the current master. The test (head, stdlib_test rubocop) failure was an unrelated Net::HTTP test on ruby-head and is now fixed there.

If you do have a case where the old code actually breaks, please tell me. That would change how we describe the change, and it would be worth a test.

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.

2 participants