Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Lua skin additions #3667

Merged
merged 2 commits into from
Aug 12, 2024
Merged

Lua skin additions #3667

merged 2 commits into from
Aug 12, 2024

Conversation

asmagill
Copy link
Member

Some proposed changes to LuaSkin:

I know we're planning a move to Swift soon, but in the meantime, this pull has the following:

  1. the pushNSObject: methods will now check for a more specific match (i.e. isMemberOfClass:) and then fall back to isKindOfClass:.

  2. the log*: methods will now accept varagrs in the NSString stringWithFormat: style.

    • this allows [LuaSkin logError:@"%s - callback error", USERDATA_TAG], as opposed to the current [LuaSkin logError:[NSString stringWithFormat:@"%s - callback error", USERDATA_TAG]]

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request does not contain a valid label. Please add one of the following labels: ['pr-fix', 'pr-change', 'pr-feature', 'pr-maintenance']

@asmagill asmagill added enhancement pr-feature Pull Request implementing a feature labels Aug 12, 2024
@cmsj
Copy link
Member

cmsj commented Aug 12, 2024

Assuming CI "passes" with only its regular amount of failing tests, this lgtm.

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 94.20290% with 4 lines in your changes missing coverage. Please review.

Project coverage is 27.95%. Comparing base (ec9a8ea) to head (e86ba5c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3667      +/-   ##
==========================================
+ Coverage   27.65%   27.95%   +0.29%     
==========================================
  Files         191      191              
  Lines       43472    43528      +56     
==========================================
+ Hits        12023    12167     +144     
+ Misses      31449    31361      -88     

@cmsj
Copy link
Member

cmsj commented Aug 12, 2024

The regular amount of tests failed, so I'm merging this.

@cmsj cmsj merged commit 45fa356 into Hammerspoon:master Aug 12, 2024
5 of 6 checks passed
@asmagill
Copy link
Member Author

Was just about to add comments in the internal pushNSNumber: method for when we migrate to Swift.

We currently do a ptr comparison against kCFBooleanTrue and kCFBooleanFalse to determine if an NSNumber is boolean (@encode for BOOL is the same as for unsigned char, so we can't use that).

Per https://stackoverflow.com/a/30223989, it's probably safer (and Swift friendly) to do something like:

func isBoolNumber(num:NSNumber) -> Bool
{
    let boolID = CFBooleanGetTypeID() // the type ID of CFBoolean
    let numID = CFGetTypeID(num) // the type ID of num
    return numID == boolID
}

They give an Obj-C version as well, but I don't want to change what's currently working atm.

If we have a place where we're sticking Swift related notes, let me know and I'll post this there.

@cmsj
Copy link
Member

cmsj commented Aug 13, 2024

We don't have such a thing yet, but we probably should. I wonder if a Project might make sense?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement pr-feature Pull Request implementing a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants