Skip to content

Minor syntactic changes#8

Merged
Leonidas-from-XIV merged 3 commits intoverbosemode:masterfrom
hannesm:minor
Oct 20, 2016
Merged

Minor syntactic changes#8
Leonidas-from-XIV merged 3 commits intoverbosemode:masterfrom
hannesm:minor

Conversation

@hannesm
Copy link
Collaborator

@hannesm hannesm commented Oct 19, 2016

No description provided.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Good idea.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Overall I think it does improve readability, thanks!

(Also, I am new to the new GitHub review thing, hope I didn't mess up too badly)

let data = String.with_range ~first:(pri_endmarker + 1) ~len:(l - pri_endmarker -1) s in
Some (facility, severity, data)
let facility = facility_of_int @@ priority_value / 8 in
let severity = severity_of_int @@ priority_value mod 8 in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could also be turned into and while at it.

match String.length s with
| l when l > 1 ->
(match String.find (fun x -> x = ' ') s with
begin match String.find (fun x -> x = ' ') s with
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big fan of this particular change as it makes it tougher for editors to match begin/end together than (/).

@Leonidas-from-XIV Leonidas-from-XIV merged commit 67c215b into verbosemode:master Oct 20, 2016
@hannesm hannesm deleted the minor branch October 20, 2016 08:10
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