Skip to content

[rb] better: allow to use devtools which are all upercase.#11912

Closed
ylecuyer wants to merge 1 commit intoSeleniumHQ:trunkfrom
ylecuyer:rb-uppercase-devtools-yle
Closed

[rb] better: allow to use devtools which are all upercase.#11912
ylecuyer wants to merge 1 commit intoSeleniumHQ:trunkfrom
ylecuyer:rb-uppercase-devtools-yle

Conversation

@ylecuyer
Copy link
Copy Markdown

@ylecuyer ylecuyer commented Apr 18, 2023

Description

This PR allows to get devtools domains which are uppercased (because there is a force capitalize atm)

Before

[38] pry(main)> driver.devtools.CSS
=> nil

Now

[2] pry(main)> driver.devtools.CSS
=> #<Selenium::DevTools::V112::CSS:0x000055a5cce00458
 @devtools=

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Couldn't check the tests as I don't have bazel installed

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 18, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@titusfortner titusfortner requested a review from p0deje April 19, 2023 03:36
@titusfortner titusfortner added the C-rb Ruby Bindings label Apr 19, 2023
p0deje
p0deje reviewed Apr 19, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

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

What is the benefit of having this? This kind of API is not idiomatic to Ruby - methods should be named lowercase and underscore.

@titusfortner
Copy link
Copy Markdown
Member

titusfortner commented Apr 19, 2023

I think the issue is our use of #capitalize means calling driver.devtools.css tries to use Selenium::DevTools::V112::Css, instead of actual Selenium::DevTools::V112::CSS. Theoretically the right Ruby answer is to use #camelCase instead of #capitalize and require driver.devtools.c_s_s.

@p0deje
Copy link
Copy Markdown
Member

p0deje commented Apr 19, 2023

Oh, I understand what the problem is now. Let me suggest a different solution - we should maintain a simple mapping of methods to classes. This can already be generated from the CDP spec and could look like this:

      METHODS_TO_CLASSES = {
        accessibility: 'Accessibility',
        animation: 'Animation',
        audits: 'Audits',
        background_service: 'BackgroundService',
        browser: 'Browser',
        css: 'CSS',
        cache_storage: 'CacheStorage',
        cast: 'Cast',
        dom: 'DOM',
        dom_debugger: 'DOMDebugger',
        event_breakpoints: 'EventBreakpoints',
        dom_snapshot: 'DOMSnapshot',
        dom_storage: 'DOMStorage',
        database: 'Database',
        device_orientation: 'DeviceOrientation',
        emulation: 'Emulation',
        headless_experimental: 'HeadlessExperimental',
        io: 'IO',
        indexed_db: 'IndexedDB',
        input: 'Input',
        inspector: 'Inspector',
        layer_tree: 'LayerTree',
        log: 'Log',
        memory: 'Memory',
        network: 'Network',
        overlay: 'Overlay',
        page: 'Page',
        performance: 'Performance',
        performance_timeline: 'PerformanceTimeline',
        security: 'Security',
        service_worker: 'ServiceWorker',
        storage: 'Storage',
        system_info: 'SystemInfo',
        target: 'Target',
        tethering: 'Tethering',
        tracing: 'Tracing',
        fetch: 'Fetch',
        web_audio: 'WebAudio',
        web_authn: 'WebAuthn',
        media: 'Media',
        device_access: 'DeviceAccess',
        preload: 'Preload',
        console: 'Console',
        debugger: 'Debugger',
        heap_profiler: 'HeapProfiler',
        profiler: 'Profiler',
        runtime: 'Runtime',
        schema: 'Schema',
      }.freeze

This way we can preserve snake_case methods while ensuring all APIs are accessible.

@p0deje p0deje closed this in e73a62b Apr 19, 2023
@p0deje
Copy link
Copy Markdown
Member

p0deje commented Apr 19, 2023

@ylecuyer Thank you for the PR. I've just pushed a different implementation, based on the previous comment. The fix will be available in the next release. Until then, later today a new nightly version of selenium-devtools and selenium-webdriver gems will be available on GitHub if you want to try using them - https://github.com/orgs/SeleniumHQ/packages?repo_name=selenium.

@ylecuyer
Copy link
Copy Markdown
Author

ylecuyer commented Apr 20, 2023

Great 👍 many thanks

@ylecuyer ylecuyer deleted the rb-uppercase-devtools-yle branch April 20, 2023 12:42
oriongonza pushed a commit to oriongonza/selenium that referenced this pull request Apr 24, 2023
@p0deje @OrionGonzalez
[rb] Maintain methods to classes map for DevTools
d2835be 
Fixes SeleniumHQ#11912
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@p0deje p0deje p0deje left review comments

Assignees

No one assigned

Labels

C-rb Ruby Bindings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ylecuyer @CLAassistant @titusfortner @p0deje