search

DarkWanderer / ClickHouse.Client

.NET client for ClickHouse
MIT License
300 stars 58 forks source link

Fix activity #291

Closed chad122 closed 1 year ago

chad122 commented 1 year ago

There is a fatal bug of the PR - add activity which is i pushed several days ago. Please merge this branch to fix it, and release a new package.

DarkWanderer commented 1 year ago

Hi,

I've merged the fix and released it. However, to prevent reoccurence - could you please add a test fixture which covers the class you added? Including the nullability cases

chad122 commented 1 year ago

Hi,

I've merged the fix and released it. However, to prevent reoccurence - could you please add a test fixture which covers the class you added? Including the nullability cases

ok

chad122 commented 1 year ago

Hi,

I've merged the fix and released it. However, to prevent reoccurence - could you please add a test fixture which covers the class you added? Including the nullability cases

I've seen your commit and i have a different idea. I think activity/trace is just an extra part. It should not throw exception to terminate the function. How about we just ignore null values instead of throw ArgumentNullException?

DarkWanderer commented 1 year ago

Hi. I appreciate your contribution and suggestions, however, as the code owner, I believe this is the right path to follow. You're welcome to raise PRs with changes to specific parts you think may benefit from it

chad122 commented 1 year ago

Hi. I appreciate your contribution and suggestions, however, as the code owner, I believe this is the right path to follow. You're welcome to raise PRs with changes to specific parts you think may benefit from it

OK