Skip to content

Commit

Permalink
fixed
Browse files Browse the repository at this point in the history
  • Loading branch information
@bararchy
bararchy committed Aug 4, 2020
1 parent e68f9d4 commit 3f32f79
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/har/cookie.cr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ module HAR
value: http_cookie.value,
path: http_cookie.path,
domain: http_cookie.domain,
expires: http_cookie.expires,
expires: http_cookie.expires.is_a?(Time) ? http_cookie.expires.as(Time) : nil,

This comment has been minimized.

Sign in to view

Copy link
@Sija

Sija Aug 4, 2020

Contributor

There's no need for that since HTTP::Cookie#expires is of a type Time? anyway.
http_only: http_cookie.http_only,
secure: http_cookie.secure
)
Expand Down Expand Up @@ -68,7 +68,7 @@ module HAR
value: value,
path: path || "/",
domain: domain,
expires: expires,
expires: expires.is_a?(Time) ? expires.as(Time) : nil,
http_only: http_only?,
secure: secure?,
)
Expand Down

7 comments on commit 3f32f79

@Sija
Copy link
Contributor

@Sija Sija commented on 3f32f79 Aug 4, 2020

Choose a reason for hiding this comment

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

That's a hack dude, not a proper fix. ;)
You probably would like to parse it (using Time.parse_iso8601 or similar) if it's a String.

@bararchy
Copy link
Member Author

Choose a reason for hiding this comment

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

@Sija I got into a situation where I get

exception: Invalid number at 0: ">>Fri, 07 Aug 2020 08:50:48 GMT" (Time::Format::Error)

Which is not ISO8601, so it's either I do analysis for each ISO, or I just say "allow it to be string, and if it's string just nil it"

@Sija
Copy link
Contributor

@Sija Sija commented on 3f32f79 Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@bararchy Yeah, but that's because you have a malformed string at your side - you should fix it before parsing it as a HAR file instead of pushing a responsibility for handling such cases to a generic lib like this.

Which is not ISO8601, so it's either I do analysis for each ISO, or I just say "allow it to be string, and if it's string just nil it"

That's not defined by any RFC or other document I know, so you just insert random business-logic into this shard.

@bararchy
Copy link
Member Author

Choose a reason for hiding this comment

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

@Sija this lib should make sure to support the common HAR generators, chrome, firefox, charles, edge, IE, opera, selenium, cypressIO, etc..
From my POV we need to make sure to parse those HARs successfully, and make sure that non-fatal issues are handled.

This seems to be a constant issue from charles, same as not putting anything in the body and instead wrap it in the "parameters" section, like chrome putting stuff automatically nilable and not by the standard, strings that can be suddenly Int, Int that becomes arrays or objects.

the HAR "rfc" is a big flaming mess, I want to make sure Crystal users can digest and handle those as best as they could.

@Sija
Copy link
Contributor

@Sija Sija commented on 3f32f79 Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@Sija this lib should make sure to support the common HAR generators, chrome, firefox, charles, edge, IE, opera, selenium, cypressIO, etc..
From my POV we need to make sure to parse those HARs successfully, and make sure that non-fatal issues are handled.

@bararchy Good point, but for that I'd recommend adding appropriate HAR files into the specs (and keeping them up to date).

the HAR "rfc" is a big flaming mess, I want to make sure Crystal users can digest and handle those as best as they could.

Fair 'nuff, but dropping values is IMO not "as best as they could".

@bararchy
Copy link
Member Author

Choose a reason for hiding this comment

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

@Sija

"as best as they could"

If you have a better idea let me know :) I just wanted that to pass and parse, the ability to create an actual cookie is less meaningful then having the actual data.

@Sija
Copy link
Contributor

@Sija Sija commented on 3f32f79 Aug 4, 2020

Choose a reason for hiding this comment

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

@bararchy Hard to come up with the actual solution without knowing the data, but I'd probably create a custom converter or sth in that vein.

Please sign in to comment.