Skip to content

fix unpack params with asasalint #642

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

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

alingse
Copy link
Contributor

@alingse alingse commented Jul 5, 2022

Development guide

Link

Test on local before making the PR

go run ./lib/utils/simple-check

How

I create a linter about pass []any as any into variadic function

the linter output

.../github/rod/must.go:870:24: pass []any as any to func Eval func(js string, args ...interface{}) *github.com/go-rod/rod.EvalOptions

@alingse
Copy link
Contributor Author

alingse commented Jul 5, 2022

I have run lint on my local, only two failed with context.deadlineExceededError

[launcher.Browser]2022/07/06 00:50:06 100%
--- FAIL: TestManaged (29.84s)
    private_test.go:117: 
        ⦗last argument⦘
        
        context.deadlineExceededError{
        }
        
        ⦗should be⦘
        
        nil
FAIL
coverage: 96.3% of statements

@alingse
Copy link
Contributor Author

alingse commented Jul 5, 2022

@ysmood need review ( and merge maybe

I also think there need a testcase maybe

@ysmood ysmood merged commit 195ec4a into go-rod:master Jul 5, 2022
@ysmood
Copy link
Member

ysmood commented Jul 5, 2022

Sorry, I accidentally merged your commit, now I reverted it, please read the source code of Page.Wait, it uses Eval already:

rod/page.go

Line 595 in c7dfaa5

opts := Eval(js, params...).ByPromise().This(this)

@ysmood
Copy link
Member

ysmood commented Jul 5, 2022

If you still want to do something, please create a unit test that can prove your idea.

@alingse
Copy link
Contributor Author

alingse commented Jul 6, 2022

what I changed is only the element MustWait
截屏2022-07-06 上午8 50 17

and Eval Signature

func Eval(js string, args ...interface{}) *EvalOptions 

how Eval used in rod repo was list here.

func (el *Element) MustEval(js string, params ...interface{}) gson.JSON {
	res, err := el.Eval(js, params...)
	el.e(err)
	return res.Value
}
---------------
func (el *Element) MustElementByJS(js string, params ...interface{}) *Element {
	el, err := el.ElementByJS(Eval(js, params...))
	el.e(err)
	return el
}
---------------
func (p *Page) MustEval(js string, params ...interface{}) gson.JSON {
	res, err := p.Eval(js, params...)
	p.e(err)
	return res.Value
}
---------------
func (p *Page) MustWait(js string, params ...interface{}) *Page {
	p.e(p.Wait(nil, js, params))
	return p
}
---------------
func (p *Page) Wait(this *proto.RuntimeRemoteObject, js string, params []interface{}) error {
	return utils.Retry(p.ctx, p.sleeper(), func() (bool, error) {
		opts := Eval(js, params...).ByPromise().This(this)
---------------

func (el *Element) CanvasToImage(format string, quality float64) ([]byte, error) {
	res, err := el.Eval(`(format, quality) => this.toDataURL(format, quality)`, format, quality)

---------------
fmt.Println("1 + 2 =", page.MustEval(`(a, b) => a + b`, 1, 2).Int())
---------------
func (p *Page) WaitElementsMoreThan(selector string, num int) error {
	return p.Wait(nil, `(s, n) => document.querySelectorAll(s).length > n`, []interface{}{selector, num})
}

the common usage by Eval is that Eval(js, params....interface{}) , and you can see
Element.MustEval and Element.MustWait and Page.MustEval

the only different is Page.Wait, it should changed, (but worked now, because finally unpack params in func body

you can see the WaitElementsMoreThan pass []interface{}{selector, num} to Page.Wait, but finally, it change to params... for Eval

so, most case Eval has no param or only one param, and the others need be unpacked

I think my change lines is ok. and I will add some unittest in another PR.

@ysmood
Copy link
Member

ysmood commented Jul 6, 2022

👍🏼

@ysmood
Copy link
Member

ysmood commented Jul 6, 2022

Also, can you help to change the Page.Wait from:

func (p *Page) Wait(this *proto.RuntimeRemoteObject, js string, params []interface{}) error

to:

func (p *Page) Wait(opts *EvalOptions) error

@alingse
Copy link
Contributor Author

alingse commented Jul 6, 2022

ok, I will try, but need go to work now 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants