Skip to content
This repository was archived by the owner on Jun 23, 2020. It is now read-only.

Update for Crystal v0.25#24

Merged
ysbaddaden merged 3 commits into
ysbaddaden:masterfrom
paulcsmith:ps-v0.25
Jun 19, 2018
Merged

Update for Crystal v0.25#24
ysbaddaden merged 3 commits into
ysbaddaden:masterfrom
paulcsmith:ps-v0.25

Conversation

@paulcsmith
Copy link
Copy Markdown
Contributor

Still has some failing tests but it compiles. I need to read up on the JSON stuff to try to figure out why it isn't passing. I probably did something wrong.

Opening this now in case you want to take it over or have some ideas.

Thanks for an awesome lib :)

@paulcsmith
Copy link
Copy Markdown
Contributor Author

Actually, it looks like the tests may be failing on master with v0.24 as well. So maybe this can be merged to get v0.25 support and then another PR for fixing the tests. For now, I'll use my forked branch in https://github.com/luckyframework/lucky_flow as it seems to work ok on v0.25 with this change :D

@bararchy
Copy link
Copy Markdown

@ysbaddaden can we get a merge on this?

@ysbaddaden
Copy link
Copy Markdown
Owner

Is there no way to keep accessing the raw objects? I really don't like JSON::Any 😢

Also the patch is hard to read because of unrelated crystal tool format changes.

@bararchy
Copy link
Copy Markdown

@ysbaddaden everyone conforms to the tool format, maybe you should too ;)

@paulcsmith
Copy link
Copy Markdown
Contributor Author

I separated commits so you can see the format changes separate from the other changes.

There may be a way to do it differently. I personally think JSON::Any is about the same, but if someone knows a better way I can update this.

@paulcsmith
Copy link
Copy Markdown
Contributor Author

e01b0fb

Still some formatting changes but mostly just stuff for JSON::Any

Comment thread src/webdriver.cr Outdated
body = JSON.parse(response.body)
status = body["status"].as_i
value = body["value"]
raise Selenium.error_class(status).new(body["value"]["message"].to_s)
Copy link
Copy Markdown
Owner

@ysbaddaden ysbaddaden Jun 17, 2018

Choose a reason for hiding this comment

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

You don't need value = body["value"] anymore and it should be body["value"]["message"].as_s.

That's part of the reason I don't like JSON::Type, it introduces confusing namings: .as_x vs .to_x.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is confusing. I'm not sure I understand the difference :S

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah that is confusing. I'm not sure I understand the difference :S

  • as_x: "type cast", type is already x
  • to_x: "type covert", type is not x (commonly)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Thank you :)

Comment thread src/webdriver/session.cr Outdated

@id = response["sessionId"].as(String)
@capabilities = response["value"].as(Hash)
@id = response["sessionId"].to_s
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

.as_s

@paulcsmith
Copy link
Copy Markdown
Contributor Author

@ysbaddaden comments have ben addressed

@ysbaddaden ysbaddaden merged commit 2279195 into ysbaddaden:master Jun 19, 2018
@ysbaddaden
Copy link
Copy Markdown
Owner

I released v0.4.0.

@paulcsmith
Copy link
Copy Markdown
Contributor Author

Awesome! Thank you :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants