Skip to content

Address #282 - improve readability of path.janet#286

Merged
bakpakin merged 1 commit intojanet-lang:masterfrom
sogaiu:expand-path-janet
Apr 6, 2026
Merged

Address #282 - improve readability of path.janet#286
bakpakin merged 1 commit intojanet-lang:masterfrom
sogaiu:expand-path-janet

Conversation

@sogaiu
Copy link
Copy Markdown
Contributor

@sogaiu sogaiu commented Apr 5, 2026

This PR is an attempt to address #282.

Apart from the original proposal, additional changes include:

  • Changes from Address #284 - handle path/normalize edge case #285 for normalize
  • Docstrings were added to satisfy assert-docs
  • A PEG was simplified (struct -> tuple)
  • PEG special usage was made more consistent (e.g. + is used everywhere instead of a mix of + and choice)
  • Some formatting was altered

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors spork/path.janet to improve readability by replacing macro-generated path functions with explicit posix/* and win32/* implementations, simplifying PEG usage, and adding docstrings to satisfy documentation checks.

Changes:

  • Removed macro-based code generation and replaced it with explicit posix/* and win32/* function definitions.
  • Simplified/standardized PEG patterns (e.g., consistent use of PEG specials like + and /).
  • Added docstrings and reworked platform-specialization bindings (sep, join, normalize, etc.).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread spork/path.janet Outdated
Comment thread spork/path.janet Outdated
Comment thread spork/path.janet Outdated
@sogaiu sogaiu force-pushed the expand-path-janet branch from f71a9f4 to e09dbc8 Compare April 6, 2026 00:31
@sogaiu sogaiu force-pushed the expand-path-janet branch from e09dbc8 to f5298e1 Compare April 6, 2026 01:18
@sogaiu
Copy link
Copy Markdown
Contributor Author

sogaiu commented Apr 6, 2026

I've made the following additional changes:

  • re: :mingw - posix?'s definition should account for "not windows and not mingw" and
  • re: comptime - delay has been used (hopefully in a correct fashion)

The result has been working here so far.

@bakpakin bakpakin merged commit 8bfdbb9 into janet-lang:master Apr 6, 2026
33 checks passed
@bakpakin
Copy link
Copy Markdown
Member

bakpakin commented Apr 6, 2026

As a side note, I realized an even more obvious way to appease linting:

(comptime (def posix? (not (index-of (os/which) [:windows :mingw]))))

It's also is more directly about intention

@sogaiu
Copy link
Copy Markdown
Contributor Author

sogaiu commented Apr 7, 2026

I tried:

(comptime (def- posix? (not (index-of (os/which) [:windows :mingw]))))

and replaced instances of (posix?) with posix? here, but now I get the "dead code" linting warning [1] from before.


[1] Under strict mode.

@bakpakin
Copy link
Copy Markdown
Member

bakpakin commented Apr 7, 2026

Right I guess that would make sense. The current version with delay is fine, just a bit ugly imo. There are several ways to do conditional compilation, and it would be nice if the most obvious ways worked with linting.

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