Skip to content

Replace 'ParsableMessage' factory methods with an initializer. #32

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 2 commits into from
Nov 8, 2019

Conversation

rxwei
Copy link
Contributor

@rxwei rxwei commented Nov 5, 2019

This PR uses an initializer init(name:kind:) to unify multiple factory methods.

This PR applies the "avoid abbreviations" naming convention to the `msg:` argument label in `ParsableMessage.beganMessage(name:msg:)`, `ParsableMessage.finishedMessage(name:msg:)` and `ParsableMessage.signalledMessage(name:msg:)`.
@rxwei rxwei requested a review from harlanhaskins November 5, 2019 22:45
@harlanhaskins
Copy link
Contributor

harlanhaskins commented Nov 6, 2019

What if we changed each of these methods to drop the redundant type name, move the message: parameter first, then drop the redundant label there too?

ParsableMessage.signalled(signalledMessage, name: job.name.rawValue)

@rxwei
Copy link
Contributor Author

rxwei commented Nov 6, 2019

I like that idea. But after giving some more thoughts to this, I think it might be better to drop these static methods and just use initializers instead, since it isn't more complicated:

ParsableMessage(name: job.kind.rawValue, kind: .signalled(message))

What do you think?

@harlanhaskins
Copy link
Contributor

That sounds great to me.

@rxwei rxwei changed the title Rename argument label 'msg:' to 'message:'. Replace 'ParsableMessage' factory methods with an initializer. Nov 6, 2019
@rxwei
Copy link
Contributor Author

rxwei commented Nov 6, 2019

Done.

@harlanhaskins
Copy link
Contributor

@swift-ci please test

1 similar comment
@rxwei
Copy link
Contributor Author

rxwei commented Nov 7, 2019

@swift-ci please test

@rxwei rxwei merged commit f5db485 into swiftlang:master Nov 8, 2019
@rxwei rxwei deleted the rename-msg-to-message branch November 8, 2019 00:50
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