search

thomthom / skippy

CLI Development tool for SketchUp extensions
MIT License
21 stars 5 forks source link

Add GitHub Actions, remove AppVeyor #50

Closed MSP-Greg closed 1 year ago

MSP-Greg commented 2 years ago

Some of the changes are from dev-appveyor branch.

See https://github.com/MSP-Greg/skippy/actions/runs/3275428680

Note that Actions will not run when added from a PR...

MSP-Greg commented 2 years ago

When working on this, I noticed that rbtree (a dependency of sorted_set) generated quite a few compile warnings with Ruby ucrt master. It would not compile with mswin master. rbtree is not a public repo on GitHub or any of the other popular online Git groups.

It seems like SortedSet is a convenience, it's not used in any 'hot paths' (called thousands of times a second, etc), so I removed it. See https://github.com/MSP-Greg/skippy/commit/178c2db623c5. Passed CI, also with a Ruby mswin job added.

Not sure whether the test suite checks its use, so not sure about another PR for it...

thomthom commented 2 years ago

It seems like SortedSet is a convenience, it's not used in any 'hot paths' (called thousands of times a second, etc), so I removed it.

Looks like SortedSet is still being used? Did you add it back in again?

thomthom commented 2 years ago

Not sure whether the test suite checks its use, so not sure about another PR for it...

I'm not sure myself, I'd have to double check.

thomthom commented 2 years ago

Thanks for having such a deep look at this. I need to spend some time myself at this PR, especially the SortedSet thing.

MSP-Greg commented 2 years ago

Looks like SortedSet is still being used? Did you add it back in again?

Sorry, you're correct. It must have been a bad day. The code to remove sorted set was in another branch based on this one. Let me cherry pick the commits back into this one...

thomthom commented 1 year ago

Fixed by #51

Thank you for taking the time to look into this!