Skip to content

Conversation

HendricksJudy
Copy link

This pull request includes several updates to improve code maintainability, fix typos, and enhance usability. The changes include making the swift command path dynamic, correcting spelling and grammatical errors in comments, fixing a typo in a test case, and updating a script to dynamically locate the swift executable.

Code maintainability and usability improvements:

  • Updated Makefile to dynamically locate the swift executable using command -v, replacing the hardcoded path. (Makefile, MakefileL21-R21)
  • Modified scripts/make-docs.sh to use command -v swift for determining the swift executable path, improving portability. (scripts/make-docs.sh, scripts/make-docs.shL42-R42)

Typo and grammar fixes:


# Commonly used locations
SWIFT := "/usr/bin/swift"
SWIFT ?= $(shell command -v swift)
Copy link
Contributor

@adityaramani adityaramani Jun 11, 2025

Choose a reason for hiding this comment

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

I think the reason this was hardcoded to explicitly use the swift wrapper binary that ships with Xcode

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert this change?


# Commonly used locations
SWIFT := "/usr/bin/swift"
SWIFT ?= $(shell command -v swift)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert this change?

fi

/usr/bin/swift package ${opts[@]}
"$(command -v swift)" package "${opts[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert this change?

@dcantah
Copy link
Member

dcantah commented Oct 6, 2025

Going to close this out, but please reopen if you have some time to revisit

@dcantah dcantah closed this Oct 6, 2025
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