Update some of Rack’s prevalent classes #232

ParadoxV5 · 2022-10-09T00:21:45Z

This PR partially addresses #210 (the updating action, not the Rack 3 concern) by reviewing and updating some of Rack’s commonly-used classes:

Builder
Config
Lobster
MediaType
MockRequest
MockResponse
Request
Response

Rack::Response was highlighted in Update RBS for Rack #210’s blockquote.
I am certain that (the signatures of) these classes were not modified from Rack 2.2.2 to 2.2.4 (Update Rack RBS to 2.2.4 #215).
The following items require a bit of follow-up:
- uses a duck type specified in Rack’s SPEC which I have not written eagerly
  - Return of Request::Helpers#body
  - Return of Request::Helpers#logger
  - Argument errors of MockResponse#initialize
- requires diving deeper into Rack’s mechanics or specifications
  - Return of Request::Helpers#session_options
  - Return of Request::Helpers#parse_multipart

I may have an interest in additionally volunteering for some middleware (but not all of the) classes after this PR. The intention is to ship the above classes first to cover most of Rack’s use cases (hopefully – I don’t wanna do the entire thing on my own 😣).

Both this and 0db9d3b are based on the Rack SPEC: https://github.com/rack/rack/blob/v2.2.2/SPEC.rdoc

inferred from source code and Rack SPEC fixes blockquote of ruby#210

I’m not entirely confident with the cookie-related entries. (I don’t work at Rack, duh.)

A note to my Rack RBS updates: I’m assuming `String` means `String` and not `#to_s` nor `#to_str` duck types (unless explicitly documented in Rack’s YARD). [It isn’t unsurprising to see a gap between official intentions and community-volunteered implementations.]

I forgot those convenience aliases I wrote, Lol.

Didn’t forget my convenience aliases this time. 🙃

got to it while doïng `Rack::Response::Helpers`, might as well tap into it

I reviewed that Blocks and Procs’ parameters are not strict, but not for Lambdas. The Lambda can just use the convenience type alias, though.

So… TypeProf’s failed attempt at expanding what might as well be `untyped` – was the cause of our lines being so long?

I’m now satisfied. 🙃 For `Helpers`, however, I need help with the following: ``` #body & #logger (no motivation to write SPEC’s duck types) #session_options (in need of more documentation for Rack) #parse_multipart (my brain raised SystemStackError) ```

This reverts commit 5d2999e.

Not marking the arg optional means the block does not need to concern the number of args (which, yeah, implicitly does not have to match). Partially reverts f68c9eb

Untyped `#body` and `#logger`: they have SPEC-specified duck types that I’m not going to write.

`No type error detected. 🧉`

ParadoxV5 · 2022-10-09T00:39:44Z

Check (link):

Skipping: cannot find test script at `gems/rack/2.2/_scripts/test`...

runs bundle exec steep check on local machine: No type error detected. 🫖

pocke · 2022-10-13T05:42:01Z

Check (link):
Skipping: cannot find test script at `gems/rack/2.2/_scripts/test`...
runs bundle exec steep check on local machine: No type error detected. 🫖

Ah, the test on CI is skipped because _scripts/test file doesn't exist. I prefer enabling CI for rack gem, so could you add this file? You can find examples from other gems, and the boilerplate generator.

pocke · 2022-10-13T06:03:56Z

I may have an interest in additionally volunteering for some middleware (but not all of the) classes after this PR. The intention is to ship the above classes first to cover most of Rack’s use cases (hopefully – I don’t wanna do the entire thing on my own 😣).

Thanks for your volunteering 🙌 I think it is not a problem that you do not write RBS for entire of the gem.
It is better if you can make a task list for classes/modules that needs updates. It will be helpful for other contributors.

pocke

This change looks good to me, thanks!

I left a comment about testing on CI. Could you confirm the comment?

ParadoxV5 · 2022-10-13T19:06:24Z

It is better if you can make a task list for classes/modules that needs updates. It will be helpful for other contributors.

Insightful suggestion! I compiled a list at #210 (comment).

P.S. I also looked at [Rack Top-Level] and RegexpExtensions during this PR's commits and found no need for changes.

ParadoxV5 · 2022-10-13T19:19:33Z

Ah, the test on CI is skipped because _scripts/test file doesn't exist. I prefer enabling CI for rack gem, so could you add this file? You can find examples from other gems, and the boilerplate generator.

Yep, I looked into it after my comment and learned that the CI check relies on _scripts/test. I tinkered a li’l bit but am not confident about where to list the newly referenced dependencies as included in 20a2571.

P.S. Do I list them in manifest.yaml as well as Steepfile?

pocke · 2022-10-15T08:36:41Z

Insightful suggestion! I compiled a list at #210 (comment).

Great! Thank you 👍 I'll share this list with colleagues who're interested in contributing to RBS.

Yep, I looked into it after my comment and learned that the CI check relies on _scripts/test. I tinkered a li’l bit but am not confident about where to list the newly referenced dependencies as included in 20a2571.

P.S. Do I list them in manifest.yaml as well as Steepfile?

Ah, good question. Currently, we need to write the dependencies to the three places, which are Steepfile, _scripts/test, and manifest.yaml. It is really inconvenient, so I'm trying to resolve this problem. Sorry for the inconvenient 🙏

By the way, only dependencies that are not in the gemspec should be written in the manifest.yaml.
rack has two dependencies, cgi and uri. Both dependencies aren't listed in the gemspec, so we need to write both gems to manifest.yml.

ruby#232 (comment)

ParadoxV5 · 2022-10-15T19:20:06Z

Inconvenient indeed, 7262031 fails because the CI tested the Rack RBS using test scripts for Active Record and Action Pack simply because those gems are out of date vs. main.

pocke

Looks good to me, thanks!

ParadoxV5 added 20 commits September 29, 2022 19:08

add convenience interface Rack::_Application

0db9d3b

update Rack::Lobster

8957a94

add convenience type-aliases env & response for Rack

3509309

Both this and 0db9d3b are based on the Rack SPEC: https://github.com/rack/rack/blob/v2.2.2/SPEC.rdoc

update RBS for Rack::Response

71feef5

inferred from source code and Rack SPEC fixes blockquote of ruby#210

update RBS for Rack::Response::Helpers

13e3602

I’m not entirely confident with the cookie-related entries. (I don’t work at Rack, duh.)

change Rack::Response#finish to use convenience alias

95f7855

I forgot those convenience aliases I wrote, Lol.

update RBS for Rack::Config

01a4050

Didn’t forget my convenience aliases this time. 🙃

update RBS for Rack::MediaType

c288645

got to it while doïng `Rack::Response::Helpers`, might as well tap into it

patch Rack Lobster::LambdaLobster & Response#each

f68c9eb

I reviewed that Blocks and Procs’ parameters are not strict, but not for Lambdas. The Lambda can just use the convenience type alias, though.

update RBS for Rack::Request::Env

c4ec2cc

So… TypeProf’s failed attempt at expanding what might as well be `untyped` – was the cause of our lines being so long?

Rack: move convenience types before constants

5d2999e

Revert "Rack: move convenience types before constants"

b53c316

This reverts commit 5d2999e.

update RBS for Rack::Builder

7d89458

patch Rack Request::Env#fetch_header & Response#each

fe1eec4

Not marking the arg optional means the block does not need to concern the number of args (which, yeah, implicitly does not have to match). Partially reverts f68c9eb

update RBS for Rack::MockRequest

3f0a838

patch Rack::Request::Helpers

c465031

Untyped `#body` and `#logger`: they have SPEC-specified duck types that I’m not going to write.

update RBS for Rack::MockResponse

9156bbe

add Rack deps introduced by Mock classes

20a2571

`No type error detected. 🧉`

ParadoxV5 mentioned this pull request Oct 9, 2022

Update RBS for Rack #210

Open

2 tasks

Merge branch 'main' into rack

8cba2f4

ParadoxV5 marked this pull request as ready for review October 9, 2022 00:29

ParadoxV5 requested a review from pocke as a code owner October 9, 2022 00:29

pocke reviewed Oct 13, 2022

View reviewed changes

ParadoxV5 added 2 commits October 15, 2022 12:46

Reïnitialize Rack (add test script)

7262031

ruby#232 (comment)

Merge branch 'main' into rack

891c748

pocke enabled auto-merge October 18, 2022 06:59

pocke approved these changes Oct 18, 2022

View reviewed changes

pocke merged commit 29592fe into ruby:main Oct 18, 2022

ParadoxV5 deleted the rack branch October 18, 2022 19:11

pocke mentioned this pull request Oct 20, 2022

Add executable permission to _scripts/test for rack #247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update some of Rack’s prevalent classes #232

Update some of Rack’s prevalent classes #232

ParadoxV5 commented Oct 9, 2022 •

edited

ParadoxV5 commented Oct 9, 2022

pocke commented Oct 13, 2022

pocke commented Oct 13, 2022

pocke left a comment

ParadoxV5 commented Oct 13, 2022 •

edited

ParadoxV5 commented Oct 13, 2022 •

edited

pocke commented Oct 15, 2022

ParadoxV5 commented Oct 15, 2022

pocke left a comment

Update some of Rack’s prevalent classes #232

Update some of Rack’s prevalent classes #232

Conversation

ParadoxV5 commented Oct 9, 2022 • edited

ParadoxV5 commented Oct 9, 2022

pocke commented Oct 13, 2022

pocke commented Oct 13, 2022

pocke left a comment

Choose a reason for hiding this comment

ParadoxV5 commented Oct 13, 2022 • edited

ParadoxV5 commented Oct 13, 2022 • edited

pocke commented Oct 15, 2022

ParadoxV5 commented Oct 15, 2022

pocke left a comment

Choose a reason for hiding this comment

ParadoxV5 commented Oct 9, 2022 •

edited

ParadoxV5 commented Oct 13, 2022 •

edited

ParadoxV5 commented Oct 13, 2022 •

edited