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.

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

Added map with status texts #214

Merged
merged 3 commits into from
Sep 7, 2022
Merged

Added map with status texts #214

merged 3 commits into from
Sep 7, 2022

Conversation

Pask423
Copy link
Contributor

@Pask423 Pask423 commented Aug 31, 2022

Base for softwaremill/sttp#1451. I would like to add set of status text here and use them in sttp core Response.

@Pask423 Pask423 requested a review from adamw August 31, 2022 16:58
@@ -2,6 +2,7 @@ package sttp.model

import internal.Validate._

// TODO: When moving to v2 add status text as field
Copy link
Member

Choose a reason for hiding this comment

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

well this models a status code, why would it include a status text? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As if I think correctly status text is to a degree part of status code as it should be same as name of status code https://developer.mozilla.org/en-US/docs/Web/API/Response/statusText

Copy link
Member

Choose a reason for hiding this comment

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

To me it looks as if it's a different property on a Response class, which can be usually calculated from the status code, but definitely not part of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fixes incoming

NetworkAuthenticationRequired -> "Network Authentication Required"
)

def get(statusCode: StatusCode): String =
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to return an Option ale let the user decide what to do if it's a non-standard status code.

Are these texts "default" descriptions, that is, might they differ in actual server responses? Why would need this map in the first place? :)

Copy link
Contributor Author

@Pask423 Pask423 Aug 31, 2022

Choose a reason for hiding this comment

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

1 OK
2 Yes I intend to use it as default.
In sttp core in Response class we allow to set status text as in the screen.
I would like to write method, in sttp core, that will use one of these texts if sttp user does not provide one

image

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so maybe let's call this method StatusTexts.default(StatusCode)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@adamw adamw merged commit c4a7bc3 into master Sep 7, 2022
@adamw
Copy link
Member

adamw commented Sep 7, 2022

Thanks :)

@mergify mergify bot deleted the status-texts branch September 7, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants