-
-
Notifications
You must be signed in to change notification settings - Fork 366
ci: Check Package.swift differences on CI #5854
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
Conversation
❌ 12 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Do I understand correctly that this script is creating a diff between the two Package.swift files and then compares it to the expected diff file, because there will always be a diff between the two files (e.g. comments)? What do you think about actually comparing the abstract syntax tree instead using e.g. SourceKitten? I am afraid the more complex the Package.swift files get over time, the more error-prone the diff will be. Also, please be more verbose with your documentation in code an PR descriptions, so others don't need to assume the reasoning for the approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in PR ⬆️
We could maybe also do it using |
@philprime, I don't think it's a good idea to add an extra dependency for this simple check. I also don't think that we will actually look at the diff. We only use it to ensure we don't update one file wihout updating the other. It's just a safety guard that you have to update whenever you touch these files. When the files deviate a lot and our current approach doesn't work, we can always revisit our approach and add something more advanced. I think this approach is good enough for now. |
If the purpose is just checking that both files are changed, then I am fine with the proposed approach. If we want to lint the actual Package.swift, I am advocating for a full validation. |
The first one, no linting. I guess we could change the workflow name to reflect this. |
The job name might have been a bad choice, the idea is to confirm the files are in sync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM with a comment.
fee877c
to
2c9251c
Compare
bda2533
to
125397f
Compare
Added a CI lint to validate the Package.swift files don't get out of sync.