Optional dataType by chkpnt · Pull Request #140 · WeTransfer/Mocker

chkpnt · 2022-11-05T15:46:03Z

As discussed in #134

I've also added an additional initializer to simplify the mock creation in case a single non-get request has to be mocked.

chkpnt · 2022-11-06T09:36:55Z

That bot seems to live in the future. 😅

wetransferplatform · 2022-11-06T09:52:58Z

	Warnings
⚠️	Consider to place some `MARK:` lines for Sources/Mocker/Mock.swift, which is over 300 lines big.

	Messages
📖	View more details on Bitrise
📖	No tests found for the current changes in build/reports

SwiftLint found issues

Severity	File	Reason
Warning	MockerTests.swift:187	Line should be 140 characters or less: currently 151 characters (`line_length`)
Warning	MockerTests.swift:205	Line should be 140 characters or less: currently 149 characters (`line_length`)
Warning	MockerTests.swift:263	Line should be 140 characters or less: currently 157 characters (`line_length`)
Warning	MockerTests.swift:52	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:71	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:94	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:116	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:148	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:173	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:189	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:207	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:230	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:283	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:544	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:578	Chained function calls should be either on the same line, or one per line. (`multiline_function_chains`)
Warning	MockerTests.swift:152	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	MockerTests.swift:167	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	Mock.swift:93	Computed properties should declare first the getter and then the setter. (`computed_accessors_order`)
Error	Mock.swift:111	Prefer checking `isEmpty` over comparing `count` to zero. (`empty_count`)
Error	Mock.swift:8	Line should be 160 characters or less: currently 177 characters (`line_length`)
Error	Mock.swift:38	Line should be 160 characters or less: currently 195 characters (`line_length`)
Warning	Mock.swift:55	Line should be 140 characters or less: currently 155 characters (`line_length`)
Error	Mock.swift:87	Line should be 160 characters or less: currently 211 characters (`line_length`)
Error	Mock.swift:90	Line should be 160 characters or less: currently 208 characters (`line_length`)
Error	Mock.swift:101	Line should be 160 characters or less: currently 217 characters (`line_length`)
Error	Mock.swift:110	Line should be 160 characters or less: currently 297 characters (`line_length`)
Warning	Mock.swift:116	Line should be 140 characters or less: currently 152 characters (`line_length`)
Warning	Mock.swift:156	Line should be 140 characters or less: currently 141 characters (`line_length`)
Error	Mock.swift:159	Line should be 160 characters or less: currently 171 characters (`line_length`)
Warning	Mock.swift:178	Line should be 140 characters or less: currently 141 characters (`line_length`)
Warning	Mock.swift:185	Line should be 140 characters or less: currently 142 characters (`line_length`)
Error	Mock.swift:186	Line should be 160 characters or less: currently 246 characters (`line_length`)
Warning	Mock.swift:204	Line should be 140 characters or less: currently 141 characters (`line_length`)
Error	Mock.swift:206	Line should be 160 characters or less: currently 171 characters (`line_length`)
Error	Mock.swift:211	Line should be 160 characters or less: currently 256 characters (`line_length`)
Warning	Mock.swift:234	Line should be 140 characters or less: currently 150 characters (`line_length`)
Error	Mock.swift:249	Line should be 160 characters or less: currently 171 characters (`line_length`)
Warning	Mock.swift:253	Line should be 140 characters or less: currently 160 characters (`line_length`)
Warning	Mock.swift:268	Line should be 140 characters or less: currently 141 characters (`line_length`)
Error	Mock.swift:270	Line should be 160 characters or less: currently 171 characters (`line_length`)
Error	Mock.swift:275	Line should be 160 characters or less: currently 262 characters (`line_length`)
Warning	Mock.swift:326	Line should be 140 characters or less: currently 159 characters (`line_length`)
Error	Mock.swift:336	Line should be 160 characters or less: currently 168 characters (`line_length`)
Warning	Mock.swift:42	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	Mock.swift:114	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	Mock.swift:155	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	Mock.swift:199	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	Mock.swift:224	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	Mock.swift:244	Lines should not have trailing whitespace. (`trailing_whitespace`)
Warning	Mock.swift:263	Lines should not have trailing whitespace. (`trailing_whitespace`)

Generated by 🚫 Danger Swift against 3b0bd4e

AvdLee

Great job, I love how you went over all code and updated the documentation as well. For a second I figured we should validate and potentially assert to have a dataType if the status code is not matching an empty response, but I figured we would take too much responsibility. I like the flexibility we give right now!

I've got one nit, and our repository requires signed commits. Would it be possible for you to enable signed commits and recommit using the following command:

 git rebase --exec 'git commit --amend --no-edit -n -S' master
 git push --force

You can read more about setting up signed commits here.

chkpnt · 2022-11-07T10:29:49Z

The commits are now signed. :-)

BasThomas

Awesome stuff, thanks @chkpnt!

AvdLee · 2022-11-15T10:27:27Z

@chkpnt thanks for signing the commits, you're almost there!

Can you finish that as well? 🙂

BasThomas

It would be amazing if we could make this a non-breaking change. Let me know if that's something I can help with. :)

chkpnt · 2022-11-15T22:19:43Z

Now we have 7 public initializers, which all need to be maintained... 🫣

I guess it's time to introduce a builder. (probably out of scope of this PR?)

BasThomas · 2022-11-16T10:43:25Z

Now we have 7 public initializers, which all need to be maintained... 🫣

I guess it's time to introduce a builder. (probably out of scope of this PR?)

🙈

Yeah, I would say that is out of scope, but an interesting idea!

BasThomas

Lookin' gooooood. Thanks so much for this, @chkpnt 🤩

A few small things I think we might want to tackle, and then I think this is good to go 👌

AvdLee

Thanks a lot for the time and effort you put into making this PR shine! 💪

BasThomas

💯

wetransferplatform · 2024-01-15T08:34:32Z

Congratulations! 🎉 This was released as part of Release 3.0.2 🚀

Generated by GitBuddy

chkpnt requested review from AvdLee, BasThomas, amirdew, kairadiagne and raphkoebraam as code owners November 5, 2022 15:46

chkpnt force-pushed the dataType branch from 4a98c8b to df54935 Compare November 6, 2022 09:41

AvdLee reviewed Nov 7, 2022

View reviewed changes

Comment thread README.md

AvdLee approved these changes Nov 7, 2022

View reviewed changes

Comment thread Sources/Mocker/Mock.swift

Comment thread Sources/Mocker/Mock.swift

chkpnt force-pushed the dataType branch 2 times, most recently from b6132a2 to bea1bf8 Compare November 7, 2022 10:28

BasThomas approved these changes Nov 14, 2022

View reviewed changes

Comment thread Sources/Mocker/Mock.swift Outdated

Comment thread Sources/Mocker/Mock.swift

BasThomas suggested changes Nov 15, 2022

View reviewed changes

Comment thread Sources/Mocker/Mock.swift

Comment thread Sources/Mocker/Mock.swift Outdated

chkpnt force-pushed the dataType branch from bea1bf8 to c4da601 Compare November 15, 2022 22:04

chkpnt requested a review from AvdLee November 15, 2022 22:20

chkpnt added 2 commits November 15, 2022 23:23

Make dataType optional

d386458

Initializer added to configure the mock using an URLRequest

aebf112

chkpnt force-pushed the dataType branch from c4da601 to d782ec8 Compare November 15, 2022 22:24

BasThomas reviewed Nov 16, 2022

View reviewed changes

Comment thread README.md

Comment thread Sources/Mocker/Mock.swift Outdated

Comment thread Sources/Mocker/Mock.swift Outdated

Comment thread Sources/Mocker/Mock.swift Outdated

Comment thread Sources/Mocker/Mock.swift Outdated

chkpnt added 2 commits November 16, 2022 17:41

Introduce new parameter contentType, which supersedes dataType

dc68f03

Do not use deprecated initializers

3b0bd4e

chkpnt force-pushed the dataType branch from 5605a77 to 3b0bd4e Compare November 16, 2022 16:41

AvdLee approved these changes Nov 17, 2022

View reviewed changes

BasThomas approved these changes Nov 17, 2022

View reviewed changes

BasThomas merged commit e5b1e27 into WeTransfer:master Nov 17, 2022

brewhitman mentioned this pull request Apr 19, 2024

Option to ignore URLs that match a prefix #157

Merged

Conversation

chkpnt commented Nov 5, 2022

Uh oh!

chkpnt commented Nov 6, 2022

Uh oh!

wetransferplatform commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SwiftLint found issues

Uh oh!

Uh oh!

AvdLee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chkpnt commented Nov 7, 2022

Uh oh!

BasThomas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AvdLee commented Nov 15, 2022

Uh oh!

BasThomas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chkpnt commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BasThomas commented Nov 16, 2022

Uh oh!

BasThomas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AvdLee left a comment

Choose a reason for hiding this comment

Uh oh!

BasThomas left a comment

Choose a reason for hiding this comment

Uh oh!

wetransferplatform commented Jan 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wetransferplatform commented Nov 6, 2022 •

edited

Loading

BasThomas left a comment •

edited

Loading

chkpnt commented Nov 15, 2022 •

edited

Loading