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

BREAKING(http): rename STATUS_CODE to STATUS_CODES and STATUS_TEXT to STATUS_TEXTS #5603

Closed
wants to merge 1 commit into from

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Aug 1, 2024

What's changed

The STATUS_CODE constant has been renamed to STATUS_CODES, and the STATUS_TEXT constant has been renamed to STATUS_TEXTS.

Motivation

The names of these constants were renamed to provide names that correctly describe what these symbols are. STATUS_CODE is not a "status code". Rather, it is an object whose entries are status codes. Similarly with STATUS_TEXT.

Also, node:http has STATUS_CODES and METHODS, which are also objects. Changing to the wrong name was made in #3781 by me.

Migration guide

To migrate, use STATUS_CODES instead of STATUS_CODE and STATUS_TEXTS instead of STATUS_TEXT.

- import { STATUS_CODE, STATUS_TEXT } from "@std/http/status";
+ import { STATUS_CODES, STATUS_TEXTS } from "@std/http/status";

- STATUS_TEXT[STATUS_CODE.NotFound]; // "Not Found"
+ STATUS_TEXTS[STATUS_CODES.NotFound]; // "Not Found"

Related

Addresses #5217 (comment)

@github-actions github-actions bot added the http label Aug 1, 2024
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 96.51163% with 3 lines in your changes missing coverage. Please review.

Project coverage is 96.38%. Comparing base (b5181c1) to head (9a466a5).

Files Patch % Lines
http/file_server.ts 85.71% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5603   +/-   ##
=======================================
  Coverage   96.38%   96.38%           
=======================================
  Files         466      466           
  Lines       37554    37554           
  Branches     5538     5538           
=======================================
  Hits        36196    36196           
  Misses       1316     1316           
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iuioiua iuioiua marked this pull request as ready for review August 1, 2024 11:38
@iuioiua iuioiua requested a review from kt3k as a code owner August 1, 2024 11:38
@kt3k
Copy link
Member

kt3k commented Aug 2, 2024

I'm not in favor of this change. The reasons are:

  • Singular form for enum like object looks just fine to me.
  • It's very late to start discussing this kind of idea (one day before the stabilization)

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

I'm strongly against this change

@BlackAsLight
Copy link
Contributor

I'm against this change. It makes sense for enums to be singular. When you get a value from it, it is only a single value in the form of dot notation. If it was more in the form of an array then I would agree it should be plural as it is an object containing status codes, but as an enum it is a status code of a value yet to be decided.

@iuioiua
Copy link
Contributor Author

iuioiua commented Aug 2, 2024

Ok. That's a sufficient amount of sentiment against. Dropping. Thanks for your input, guys.

@iuioiua iuioiua closed this Aug 2, 2024
@iuioiua iuioiua deleted the http-status-codes-texts branch August 2, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants