Tidy up HTTP routes by using locally scoped imports

RESOLVED INVALID

Status

enhancement
P2
normal
RESOLVED INVALID
2 months ago
a month ago

People

(Reporter: ato, Unassigned)

Tracking

({good-first-bug})

Version 3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 months ago

This is a bit of tidying up bug, for no other reason than making
the code nicer to read.

testing/webdriver/src/httpapi.rs installs HTTP request routing
we need for the WebDriver server, but it’s awfully messy because
of rustfmt(1)’s formatting rules. To ease the readability a little
bit we can import Method::GET, ::DELETE, ::POST, and all the
routes using a use statement at the top of the function:

use Method::{GET, POST, DELETE};
use Route::*;
…
(Reporter)

Updated

2 months ago
Keywords: good-first-bug
(Reporter)

Comment 1

2 months ago

There are also some extension routes defined in geckodriver that
can be cleaned up:
https://searchfox.org/mozilla-central/rev/93905b660fc99a5d52b683690dd26471daca08c8/testing/geckodriver/src/command.rs#19-57

But since they belong to a different component in the tree, they
belong in a separate commit than the changes to the webdriver crate.

Comment 2

2 months ago

Hey!
Can I take up this bug?

(Reporter)

Comment 3

2 months ago

Yes, please see
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
on how to contribute code to Gecko. There is geckodriver specific
documentation at
https://firefox-source-docs.mozilla.org/testing/geckodriver/index.html#for-developers.

(Reporter)

Updated

2 months ago
Priority: -- → P2

Comment 4

a month ago

Hi I would like to take this up in case it is not active. Please let me know if I may. Thanks!

(Reporter)

Comment 5

a month ago

First person to submit a patch wins, generally.

Comment 6

a month ago

:ato I have a couple doubts. I researched a bit around http requests in rust and the use of associated libraries built over it.
Going with hyper, the Method is an implementation with GET, POST, DELETE as its constant functions, importing which directly, throws the accessibility error of private variants. So I think of building a wrapper around each of the three requests as three functions, calling which will return the desired method and reduce the verbosity of the code. Is this approach fine or am I missing something?

(Reporter)

Comment 7

a month ago

That sounds overly complicated. The scope of this task is to move
the use Method::*; statement from the global scope, into the
function scope so that route declarations such as

(Method::POST, "/session", Route::NewSession),

can be shortened to

(POST, "/session", Route::NewSession),

Does that help simplify things?

(And, of course, the same principle can be applied to Route so
that Route::NewSession becomes NewSession.)

(Reporter)

Comment 9

a month ago

It turns out Method is a struct, and they are not use-able.

Status: NEW → RESOLVED
Last Resolved: a month ago
Resolution: --- → INVALID

Comment 10

a month ago

Since this patch does not use locally scoped imports but does perform the tidying up job in the HTTP routes, I was wondering if it could actually constitute the solution to a new bug that demands tidying up without expecting the locally scoped imports approach? Would that be the case :ato ?

(Reporter)

Comment 11

a month ago

(In reply to Kriti Singh from comment #10)

Since this patch does not use locally scoped imports but does
perform the tidying up job in the HTTP routes, I was wondering
if it could actually constitute the solution to a new bug that
demands tidying up without expecting the locally scoped imports
approach? Would that be the case :ato ?

I’m not sure the locally scoped GeckoExtensionCommands warrant a
change, but I guess I wouldn’t reject a patch to that effect.

You need to log in before you can comment on or make changes to this bug.