Skip to content

Conversation

kaylendog
Copy link
Contributor

@kaylendog kaylendog commented Oct 8, 2025

Move room name, avatar, and topic to IOpts, over using ICreateRoomOpts. This will facilitate the logic in #30877.

Checklist

@kaylendog
Copy link
Contributor Author

I'm not sure if it's worth addressing the coverage issues in this PR, since as far as I can tell it's related to the underlying code not having good coverage.

@kaylendog kaylendog marked this pull request as ready for review October 9, 2025 15:09
@kaylendog kaylendog requested a review from a team as a code owner October 9, 2025 15:09
@t3chguy
Copy link
Member

t3chguy commented Oct 9, 2025

I'm not sure if it's worth addressing the coverage issues in this PR, since as far as I can tell it's related to the underlying code not having good coverage.

When would the underlying code get good coverage then though? We normally mandate that refactors and the like do have to bring the coverage up to scratch otherwise it'll never be done

@kaylendog
Copy link
Contributor Author

I'm not sure if it's worth addressing the coverage issues in this PR, since as far as I can tell it's related to the underlying code not having good coverage.

When would the underlying code get good coverage then though? We normally mandate that refactors and the like do have to bring the coverage up to scratch otherwise it'll never be done

It just seems a bit silly to write tests for existing uncovered code that I haven't modified in an otherwise unrelated refactoring PR. To me, it'd make more sense for such tests to go in their own PR dedicated explicitly to improving coverage. If it's mandated, however, I'll attempt to write some tests for it and include them here.

@t3chguy
Copy link
Member

t3chguy commented Oct 9, 2025

We don't have a way of bypassing the sonarcloud coverage gate without also bypassing a bunch of merge queue tests (until #30630 lands) so we are quite averse to bypassing it as it means a plethora of tests won't get executed. I doubt you want to block on #30630 landing first though

@kaylendog
Copy link
Contributor Author

We don't have a way of bypassing the sonarcloud coverage gate without also bypassing a bunch of merge queue tests (until #30630 lands) so we are quite averse to bypassing it as it means a plethora of tests won't get executed. I doubt you want to block on #30630 landing first though

That's fair. I'll commit to writing tests here then.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants