chore: error handling in params construction

[kaichaosun]

Description

The change add error handling based on this comment #930 (comment)
This seems a bit tedius to me and brings less value. How do you think?

[status-im-auto]

Jenkins Builds

Click to see older builds (5)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	0306f5b	#1	2023-11-29 10:34:40	~1 min	nix-flake	📄log
✔️	249d097	#2	2023-11-30 02:24:15	~1 min	nix-flake	📄log
✔️	259a516	#3	2023-11-30 02:27:35	~1 min	nix-flake	📄log
✔️	ac3e8e3	#4	2023-11-30 02:29:30	~1 min	nix-flake	📄log
✔️	7a3b601	#5	2023-11-30 06:17:40	~1 min	nix-flake	📄log

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	7f9f9f2	#6	2023-11-30 06:20:41	~1 min	nix-flake	📄log
✔️	09b8812	#7	2023-12-01 03:19:50	~1 min	nix-flake	📄log

[richard-ramos]

I like the change. It will for sure improve the dev experience, since then a developer will know for sure that they cannot use both options at the same time (before they could but then the nameservers would have been ignored, so this will potentially save them debugging time and for us, having to give support in case such scenario happens)

[kaichaosun]

Agree with the dev ex. I added another test for the coverage, please give another look. @chaitanyaprem @richard-ramos

[chaitanyaprem]

LGTM

[kaichaosun]

codeclimate check is failed with excessive return statement, I loose the check for 6 return statements. I can also refactor the code to reduce the return statement, but have 4 return statement looks pretty strict.

Any clue why test-ci failed? It's happy now. 😂

[chaitanyaprem]

codeclimate check is failed with excessive return statement, I loose the check for 6 return statements. I can also refactor the code to reduce the return statement, but have 4 return statement looks pretty strict.

Any clue why test-ci failed? It's happy now. 😂

Codeclimate check is not mandatory as it has some weird checks like this return statements which are not possible to enforce all the time.

test-ci failure could be due to a flaky test, have noticed this sometimes before as well.

But, I don't think increasing return statements to 6 helps much, i would suggest if we can make codeclimate ignore this in total. @richard-ramos WDYT?

[richard-ramos]

Any clue why test-ci failed? It's happy now.

Probably due to the flaky postgresql tests

[richard-ramos]

But, I don't think increasing return statements to 6 helps much, i would suggest if we can make codeclimate ignore this in total. @richard-ramos WDYT?

We have 3 options here:

1. Disable it
2. Mark as wont fix in code climate

I kinda like option 2 because having multiple returns could be a sign of code smell, or a function that needs to be split up.

[chaitanyaprem]

But, I don't think increasing return statements to 6 helps much, i would suggest if we can make codeclimate ignore this in total. @richard-ramos WDYT?

We have 3 options here:

1. Disable it

2. Mark as wont fix in code climate

I kinda like option 2 because having multiple returns could be a sign of code smell, or a function that needs to be split up.

Let's go with option-2. Even i like it better.

@kaichaosun , please revert the codeclimate check change and then you are good to merge this PR.