Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[feat] improve type safety for Named, servantify brig internal route #3634

Merged
merged 6 commits into from
Oct 9, 2023
Merged

[feat] improve type safety for Named, servantify brig internal route #3634

merged 6 commits into from
Oct 9, 2023

Conversation

MangoIV
Copy link
Contributor

@MangoIV MangoIV commented Oct 6, 2023 •
edited
Loading

https://wearezeta.atlassian.net/browse/WPB-1224

  • improve type safety of Named by making it possible to rule out weakly typed arguments to the type (e.g. Type)
  • servantify the internal route for querying the teams API for servant

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 6, 2023
@MangoIV MangoIV force-pushed the mangoiv/servantify/brig-team-api branch 2 times, most recently from cc3e290 to f4814d2 Compare October 7, 2023 11:26
@MangoIV MangoIV marked this pull request as ready for review October 7, 2023 14:05
elland
elland reviewed Oct 9, 2023
View reviewed changes
Copy link
Contributor

@elland elland left a comment

Choose a reason for hiding this comment

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

How do you feel about moving the api handler definitions to the same file where we define the routes?

libs/wire-api/src/Wire/API/Routes/Internal/Brig.hs Outdated Show resolved Hide resolved
libs/wire-api/src/Wire/API/Routes/Named.hs Show resolved Hide resolved
@MangoIV
Copy link
Contributor Author

MangoIV commented Oct 9, 2023

How do you feel about moving the api handler definitions to the same file where we define the routes?

@elland the file I would move to is already quite large and the Brig.Team.API file contains the public routes as well, so that was my reason to not move it, if you wish though, I will move it over.

Maybe also consider moving the API definition itself to Brig.Team.API, similarly how it is done with the exported servantAPI

@MangoIV MangoIV force-pushed the mangoiv/servantify/brig-team-api branch 2 times, most recently from 293b6f6 to 5174695 Compare October 9, 2023 11:35
@MangoIV MangoIV requested a review from elland October 9, 2023 12:43
@MangoIV MangoIV force-pushed the mangoiv/servantify/brig-team-api branch from 8d446af to 5174695 Compare October 9, 2023 13:47
MangoIV and others added 6 commits October 9, 2023 17:31
@MangoIV
[feat] improve type safety for Named, servantify brig internal route
6b3433e 
- improve type safety of Named by making it possible to rule out
  weakly typed arguments to the type (e.g. Type)
- servantify the internal route for querying the teams API for servant
@MangoIV
[fix] change status code of servantified route from 204 to 200 where …
eadd2eb 
…needed
@MangoIV
[chore] add changelog.d entry
039ff0f
@MangoIV
[fix] revert unwanted formatting changes
a68e625
@MangoIV
[fix] apply changes suggest by elland
c84b5e2 
- add documentation of `Named` changes
- s/UnSuspendTeam/UnsuspendTeam
@MangoIV
[fix] fix the schema
b9ced29
@MangoIV MangoIV force-pushed the mangoiv/servantify/brig-team-api branch from 5174695 to b9ced29 Compare October 9, 2023 15:32
@MangoIV
Copy link
Contributor Author

MangoIV commented Oct 9, 2023

finally the tests have passed!

@MangoIV MangoIV merged commit 903b87e into develop Oct 9, 2023
9 checks passed
@MangoIV MangoIV deleted the mangoiv/servantify/brig-team-api branch October 9, 2023 16:32
This was referenced Oct 19, 2023
Closed
Merged
@echoes-hq echoes-hq bot added the echoes: technical-debt Changes intended at mitigating risks label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elland elland elland approved these changes

Assignees
No one assigned
Labels
echoes: technical-debt Changes intended at mitigating risks ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MangoIV @elland @zebot