Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GitHub actions #10

Merged
merged 4 commits into from
Dec 29, 2024
Merged

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
Owner

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
Owner

@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
Owner

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 johnmai-dev:main Dec 29, 2024
2 checks passed
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