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

#421 fix trailing comma #422

Merged
merged 4 commits into from
May 14, 2024

Conversation

mostazomarc
Copy link
Contributor

Contributor checklist


Description

Remove trailing_comma as a disabled rule from .swiftlint.yaml and fixed all errors for it in the codebase.

The linter fix was tested running: swiftlint --no-cache --config ~/com.raywenderlich.swiftlint.yml
and tested on:

  • iPhone 15 Pro Max, iOS 17.4. (simulator)

Related issue

Copy link

github-actions bot commented May 5, 2024

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and iOS rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-iOS repo
  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@mostazomarc
Copy link
Contributor Author

The SwiftLint pull request workflow is checking for errors that it shouldn't be, right?

@andrewtavis
Copy link
Member

Seems like it, @mostazomarc :) There was another implementation of the workflow from #417, but then it made sense to me to just use the pre made action 🤔

Thanks for the contribution! Let's see if we can figure this out :)

@andrewtavis
Copy link
Member

Looking at the workflow, seems like we're missing the paths options at the top? Do you want to add those and see if it works? Note that we have .yaml instead of .yml. Feel free to experiment with any of the other options you see that you think might help!

@mostazomarc
Copy link
Contributor Author

Sure! I'll go ahead and add the paths option at the top of the workflow file, I'll make sure to use .yaml instead of .yml.

@mostazomarc
Copy link
Contributor Author

Paths options didn't solve the problem.
I believe norio-nomura/[email protected] uses a config file named .swiftlint.yml (as it worked okay on #420) and it was renamed to .swiftlint.yaml in this commit so it no longer uses the configuration provided?
Maybe I could try renaming .swiftlint.yaml back to .swiftlint.yml or some workaround?

@andrewtavis
Copy link
Member

Trying to rename the file to .yml would be fine :) We can also rename the workflow to a .yml file as in the example. We normally use the longer version of the file extension, but it that's what's needed, then no stress at all 😊

@andrewtavis
Copy link
Member

So should be fine to just switch it back to .yml as you mentioned that it will work like #420. But maybe let's keep the path definition in there so it's a bit more verbose?

@mostazomarc
Copy link
Contributor Author

I actually don't know why is doing all these checks as they should be disabled by the .swiftlint.yml file.
I thought it was because of the .yaml file extension, but it seems not.

@andrewtavis
Copy link
Member

Ya I'm really not sure what's up... As a final check, let's maybe set it back to the state of the last passed tests via the branch for it. Differences there are:

  • The workflow is .yaml
  • There are no paths in the workflow

Would you want to reset it so we can at least confirm that there's nothing new going on? Sorry for not being a bit more active on trying things. I'm currently traveling 🌴 Appreciate all the effort going into this!

@mostazomarc
Copy link
Contributor Author

Sure! I'll reset the last two commits while we figure out what's going on.

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Looks like we just need to keep the file as swiftlint.yml, @mostazomarc 😊 Appreciate the support here! Let me know if you have any interest in working on the other linting rules. Would be super happy to make some more issues for this, or we could also check in on making some issues together for yourself and others to work on!

@andrewtavis andrewtavis merged commit 12537ce into scribe-org:main May 14, 2024
1 check 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