Fixes to the Test Suite, and a Dependency Refactor#147
Open
kkohrt wants to merge 7 commits intorails:masterfrom
Open
Fixes to the Test Suite, and a Dependency Refactor#147kkohrt wants to merge 7 commits intorails:masterfrom
kkohrt wants to merge 7 commits intorails:masterfrom
Conversation
1afba33 to
1e55731
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A recent cleanup PR removed a redundant accessor from a file (module.rb) that actually used it, which made it hard to follow the code path.
It was ultimately a final step in a chain of cleanup actions in execjs.rb.
This PR basically unwinds a much earlier refactor of execjs.rb here that was written so that:
But that same problem was re-addressed here in order to:
which now left module methods defined in two places :-(
This PR
Glues it all back together again into an single module definition
Fixes some predicate methods to return
false, notnilFixes a warning in the test suite
Uses File.join rather than concatenating strings for path definitions (since Windows is supported?)
Does some quick fixes on spec tests and CI config to get everything to pass ✅
These could probably use some additional refinement, but
I don't have the setup to do it locallyhey! I think I got github actions running in my fork. I will try to tighten up the code wherever I can in the coming days; and happy to take any suggestions in the meantime.NOTE:
A future TODO item would be to clean up this section of ExternalRuntime so that either (a) the switchard is inside a lightweight
external_execmethod than then calls either static or dynamically created platform-specific methods to actually do the work; or (b)external_execis defined only when needed at runtime usingmethod_missing, and again, executes the switchyard logic at that time rather than when the class is loaded.