Skip to content

EthereumContractTest removed swiftLint errors and warnings #688

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

albertopeam
Copy link
Contributor

removed:

  1. error: Force Cast Violation
  2. error: Force Try Violation
  3. warning: Force Unwrapping Violation
  4. warning: Indentation Width Violation

@albertopeam
Copy link
Contributor Author

It seems a good idea(at least for me) to start cleaning up tests from swiftLint warns and errors, small PRs that are easy to read.
what do you think?

@janndriessen
Copy link
Collaborator

It seems a good idea(at least for me) to start cleaning up tests from swiftLint warns and errors, small PRs that are easy to read. what do you think?

This is a good idea! I think it would be best though to wait until #685 #683 #673 are all merged - as otherwise this will just lead to lots of merge conflicts.

@yaroslavyaroslav
Copy link
Collaborator

@albertopeam just saying, that by far we've discussed such, though it was a long time ago, we doesn't apply such sever rules for tests as for the library code itself, for example there was force try and force unwrap allowing as long as it passed ci/cd pipeline.

This is not that I'm against such sever rules for tests I'm actually up to, just back there there was a very few folks who contribute here constantly besides me, drastically in contrary as it is now, but my point is that it worth to at least discuss and vote for all such point which in whole would become contribution guidelines actually.

@pharms-eth
Copy link
Collaborator

It seems a good idea(at least for me) to start cleaning up tests from swiftLint warns and errors, small PRs that are easy to read. what do you think?

This is a good idea! I think it would be best though to wait until #685 #683 #673 are all merged - as otherwise this will just lead to lots of merge conflicts.

in addition to:
#689
#687
thank you

@albertopeam
Copy link
Contributor Author

@albertopeam just saying, that by far we've discussed such, though it was a long time ago, we doesn't apply such sever rules for tests as for the library code itself, for example there was force try and force unwrap allowing as long as it passed ci/cd pipeline.

Sure, I am here to listen and learn. If you want to discuss the rules of swiftlint I am in :)

@albertopeam albertopeam force-pushed the feature/remove-swiftlint-warnings-EthereumContractAddress branch from 2ab41c0 to 38c3de4 Compare December 31, 2022 17:15
@albertopeam
Copy link
Contributor Author

albertopeam commented Dec 31, 2022

my point of view is to avoid using ! in code and also in tests. Reason:
I can understand that maybe is not so important in tests but the thing is that a ! will crash the test suite, I think the proper way is to not crash to fail elegantly. To do it is kind of easy to replace force unwrap !with XCTUnwrap.
Said that, I can do it progresively file by file in small PRs that can be quickly merged if you think my point is valid or better than now :)

Copy link
Collaborator

@janndriessen janndriessen left a comment

Choose a reason for hiding this comment

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

LGTM!

@yaroslavyaroslav yaroslavyaroslav merged commit cc119c4 into web3swift-team:develop Jan 10, 2023
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.

4 participants