Skip to content

Fix GitHub actions#10

Merged
johnmai-dev merged 4 commits intohuggingface:mainfrom
DePasqualeOrg:fix-github-actions
Dec 29, 2024
Merged

Fix GitHub actions#10
johnmai-dev merged 4 commits intohuggingface:mainfrom
DePasqualeOrg:fix-github-actions

Conversation

@DePasqualeOrg
Copy link
Collaborator

@DePasqualeOrg DePasqualeOrg commented Dec 26, 2024

The current GitHub actions appear to be failing, maybe because they include Swift 5.8. I also included a swift-format step.

@DePasqualeOrg
Copy link
Collaborator Author

DePasqualeOrg commented Dec 26, 2024

Now this works.

Regarding the formatting rules: Would you mind if we remove lineBreakBeforeControlFlowKeywords? I find the code more difficult to read with this setting enabled, and I think most Swift code does not use this rule, but it's also a matter of personal taste.

@DePasqualeOrg DePasqualeOrg mentioned this pull request Dec 26, 2024
@DePasqualeOrg
Copy link
Collaborator Author

I added the same pre-commit hook as in mlx-swift, so that you can run the following command to format before committing: pre-commit run --all-files

@johnmai-dev johnmai-dev self-requested a review December 26, 2024 13:37
@johnmai-dev
Copy link
Collaborator

Now this works.

Regarding the formatting rules: Would you mind if we remove lineBreakBeforeControlFlowKeywords? I find the code more difficult to read with this setting enabled, and I think most Swift code does not use this rule, but it's also a matter of personal taste.

You can remove it.

@DePasqualeOrg
Copy link
Collaborator Author

Okay, I think this is ready to go.

@DePasqualeOrg
Copy link
Collaborator Author

@johnmai-dev, once you're able to merge this, I'll rebase my previous PR to make it easier for @pcuenca to review it.

Copy link
Collaborator

@johnmai-dev johnmai-dev 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 the fix!

Comment on lines +27 to +29
- name: Select Xcode
run: sudo xcode-select -s /Applications/Xcode_${{ matrix.xcode }}.app

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove Select Xcode? Previously, it was needed to specify xcode for version 5.8. @DePasqualeOrg

@johnmai-dev johnmai-dev merged commit 10cf426 into huggingface:main Dec 29, 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.

2 participants