Skip to content
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

Merge perfaram/master into beltex/swift2 #21

Closed
wants to merge 11 commits into from
Closed

Merge perfaram/master into beltex/swift2 #21

wants to merge 11 commits into from

Conversation

perfaram
Copy link
Contributor

Fixes #17 and follows #20

@beltex
Copy link
Owner

beltex commented Jun 25, 2015

Hi @perfaram!

Thanks for the pull request, its much appreciated, and the first of the project too! :)

This is neat work, but I wanted to take a bit of a different approach. While the underlying mechanism would be using the index selector, wanted to stick to primarily temperature keys only for now. So something like unknownTemperatureKeys() -> [String]. Possibly a allKeys() -> [String] as well, but only returning the FourCC. In addition, I wanted to get some refactoring in first on the Swift 2.0 branch.

Thus, I will not be merging this. However, I’ll note some feedback nonetheless for future reference.

@@ -617,6 +662,8 @@ public struct SMC {
- parameter key: The SMC key to check. 4 byte multi-character constant. Must be
4 characters in length.
- returns: valid True if the key is found, false otherwise
- returns: IOReturn IOKit return code
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like these got added back in. I removed them because originally, way back, I had thought there should be a :returns: line for each return value, since tuple returns were new to Swift. However, single return line is the convention it seems, independent of the number of values.

@perfaram perfaram closed this Jun 25, 2015
@perfaram
Copy link
Contributor Author

So, thanks for having looked over the modifications even if you didn't mean to merge. Thus, thanks also for the tips.
One more thing : I'll fix some things in my repo and then keep it up-to-date with yours, so that whenever you want to start support for non-temp keys, there will be pieces of code ready (such as value decoding).

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