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.

Sign up for GitHub

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

Jump to bottom

Silence warnings from 3rd-party CocoaPods dependencies #6196

Merged
merged 5 commits into from
Jun 1, 2022
Merged

Silence warnings from 3rd-party CocoaPods dependencies #6196

merged 5 commits into from
Jun 1, 2022

Conversation

wtimme
Copy link
Contributor

@wtimme wtimme commented May 23, 2022 •
edited
Loading

Introduction

This pull request configures CocoaPods in the way that warnings from 3rd-party code are not displayed in Xcode. This has two huge benefits:

  • Seeing the number of warnings going down is a very, very nice feeling
  • We only see warnings for things that we can fix, right there, in our code base

Here's a clean build on my machine:

develop right now This branch
element-warnings-before element-warnings-after

The one instance that one might want to see warnings is when using a "local" version of the matrix-ios-sdk. I was unsure whether this is something that you want/need, so I did not add it. Should not be much effort though, so let me know what you think.

Signed-off-by: Wolfgang Timme element-signoff@milchbartstrasse.de

Pull Request Checklist

  • I read the contributing guide
  • UI change has been tested on both light and dark themes, in portrait and landscape orientations and on iPhone and iPad simulators
  • Accessibility has been taken into account.
  • Pull request is based on the develop branch
  • Pull request contains a changelog file in ./changelog.d
  • You've made a self review of your PR
  • Pull request includes screenshots or videos of UI changes
  • Pull request includes a sign off

@wtimme
Prevent warnings from 3rd-party code to show up in Xcode
8a678ba 
The warnings from 3rd-party code can mostly be ignored, because it is not
under the control of our team. In Xcode, this allows us to focus on
the issues that we are able to fix.
@wtimme
Remove redundant "inhibit warnings" direction
a7d55a0 
Since all warnings are inhibited by default, there is no need to
manually set this parameter here to `true`.
@pixlwave
Copy link
Member

Hey @wtimme thanks for taking the time to open this PR, it sound great. As you mentioned it would indeed be nice for us to still see warnings for MatrixSDK as we are responsible for that. Additionally it would be nice to see them for AnalyticsEvent as that's one of ours and I was thinking about Olm too but I see they're already disabled on that due to it being "bad" so probably a good idea to leave that alone 😅

Additionally would you be able to a) add a sign off to your PR and b) update the changelog filename to pr-6196.build as it doesn't have an issue.

@wtimme
Add changelog entry for #6196
d039561
@wtimme
Let warnings from MatrixSDK and AnalyticsEvents show up in Xcode
95387a6 
As requested by @pixlwave during their review of #6196, these
`inhibit_warnings` directive configure CocoaPods in a way that the
warnings for both MatrixSDK and AnalyticsEvents show up when building
the workspace in Xcode.
@wtimme
Copy link
Contributor Author

wtimme commented May 31, 2022

Thanks for your feedback, @pixlwave! I have updated the pull request's description to include a Sign-Off as per your comment.

I have also added :inhibit_warnings => false for the dependencies MatrixSDK and AnalyticsEvents.

Here's another comparison after updating the Podfile:

develop (before) this pull request's branch
element-warnings-develop element-warnings-after

@wtimme wtimme changed the title Silence about 200 warnings from CocoaPods dependencies Silence warnings from 3rd-party CocoaPods dependencies May 31, 2022
pixlwave
pixlwave approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Looks good to me. Sorry, there's a conflict in the Podfile now that we've just made a new release. If you would like to update that I can merge this afterwards :)

@wtimme
Copy link
Contributor Author

wtimme commented May 31, 2022

Thanks for your approval, Doug! I have merged develop into my branch, so as soon as the GitHub workflows are done, this pull request could be merged.

@pixlwave pixlwave merged commit 90041a6 into element-hq:develop Jun 1, 2022
@pixlwave
Copy link
Member

pixlwave commented Jun 1, 2022

Thanks for your contribution Wolfgang 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pixlwave pixlwave pixlwave approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wtimme @pixlwave