-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Filedownloads url - Adhere to w3c standards #11646
Filedownloads url - Adhere to w3c standards #11646
Conversation
This PR takes care of ONLY streamlining the url to be compliant with The PR for #11443 will ensure that the downloads path references are all removed. I thought it would be good if I go at this requirement on a piece by piece |
Codecov ReportBase: 54.65% // Head: 54.65% // No change to project coverage 👍
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## trunk #11646 +/- ##
=======================================
Coverage 54.65% 54.65%
=======================================
Files 85 85
Lines 5643 5643
Branches 244 244
=======================================
Hits 3084 3084
Misses 2315 2315
Partials 244 244 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Hi Community, |
@krmahadevan I agree with going piece by piece, so I broke up the issue above into 5 components, this being two I think. Will review soon. @surajverma255 welcome! Please read the issues linked in the PR to better understand the context. |
yeah, ok, @diemol will have to review the code as I'm lost in this part of Selenium, but the syntax looks correct to me. Diego previously proposed to add file timestamp info with the GET response. To be honest, it would be a lot easier not to include it. Especially if we are already breaking things out by session, but we should agree on that bit. |
4c7d2d4
to
92c9650
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just very minor comments, as in the endpoint already shows it is about files.
Thank you, @krmahadevan!
92c9650
to
e25bfae
Compare
Thank you @diemol. i have fixed the review comments as an additional commit and also rebased off of |
@krmahadevan sure, go for it. |
Fixes: SeleniumHQ#11466, SeleniumHQ#11458 New format `POST /se/files` Request: `{ "filename": "file" }` Response: ``` { "value": { "filename": "filename", "contents": "ContentsGoHere" } } ``` `GET /se/files` Request: NONE Response: ``` { "value": { "files":[ "1", "2", "3" ] } } ```
e25bfae
to
63af6a8
Compare
Fixes: #11466 #11458
New format
POST /se/files
Request:
{ "filename": "file" }
Response:
GET /se/files
Request:
NONE
Response:
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Motivation and Context
Types of changes
Checklist