Closed mattathompson closed 6 years ago
Looks like it's broke on windows as of right now.
@gantman, the appVoyer runner is for windows? That makes much more sense.
The issue is around the use of the spread operator. I'll look into windows/node limitations.
It's a problem introduced upstream in gluegun
with the new table stuff.
Is Appveyor targeting < 8.6 of Node. If so, then great. We should have checks like this up on gluegun
.
Ok, I have PR in place now. We'll get this merged in shortly and cut a new build of gluegun
.
Thanks for picking that up @skellock
:exclamation: No coverage uploaded for pull request base (
master@d55a791
). Click here to learn what that means. The diff coverage is95.28%
.
@@ Coverage Diff @@
## master #134 +/- ##
=========================================
Coverage ? 81.54%
=========================================
Files ? 28
Lines ? 531
Branches ? 83
=========================================
Hits ? 433
Misses ? 66
Partials ? 32
Impacted Files | Coverage Δ | |
---|---|---|
src/types.ts | 100% <ø> (ø) |
|
...c/extensions/functions/appendSolidaritySettings.ts | 100% <100%> (ø) |
|
src/commands/snapshot.ts | 52.27% <90.9%> (ø) |
|
src/extensions/functions/ruleHandlers.ts | 93.93% <93.93%> (ø) |
|
...c/extensions/functions/buildSpecificRequirement.ts | 94.73% <94.73%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update d55a791...4af9018. Read the comment docs.
^ having trouble actually adding a file rule.
That error message I added to your code, to identify the issue.
Also, looks like it's overwriting and not appending:
Yeah, I saw that when writing more specs this evening. I've got a fix for that but, I'm working through some test failures.
@mattathompson - cool bud! Let me know when you want me to test again 👍
Yeah, I haven't had as much time as I'd like to work on opensorce this week. I'll ping you when it's ready.
On Nov 29, 2017 10:27 PM, "Gant Laborde" notifications@github.com wrote:
@mattathompson https://github.com/mattathompson - cool bud! Let me know when you want me to test again 👍
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/infinitered/solidarity/pull/134#issuecomment-348071903, or mute the thread https://github.com/notifications/unsubscribe-auth/AF83KwWub59U-rgZ8AnkxZVyAM99ycN-ks5s7iCcgaJpZM4QqhGY .
@GantMan: Playing around with the new functionality I've noticed one use case of interest:
Adding a rule that already exists in the solidarity file... Should we handle that? I built something to handle it w/ cli rules but, would need to extend to all of the other rules.
Or.. Should we just let the user add rules regardless of the fact that they already exists? If so i can just remove that code.
Just pulled latest, here's my report:
solidarity snapshot cli
but forgets to mention the CLI they want, we can just ask with an extra question. "What's the name of the CLI you'd like to add a rule for?" solidarity snapshot node
we should give a friendly error. "node
is not a known solidarity rule. To add a rule via snapshot you can type solidarity snapshot <rule> <item>
"Your new requirement was not added.
I think replacing would be best.
This PR is starting to really shape up. Let me know if I can help in any way 👍
Heya @mattathompson - just got back from a biz trip. Anything I can do to help?
@GantMan, I'm working on this today. I've addressed all the issues but one so far. Currently right now if you add a duplicate you end up just having two of the same rule.
Thinking through a solution for that.
Awesome bud. Let me know if I can help in any way.
Btw I’m presenting this lib at a meetup on the 19th! I’ll be giving you a shoutout
Hey Matthew Thompson, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
@GantMan Ready for Review. Need to update the documentation but, this is good to test drive/ code review.
Hey Matthew Thompson, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey Gant Laborde, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
We've been getting this random failure ^ Not sure what this is about. I've seen it now twice in different tests. Have not seen it locally.
I believe it's a timeout issue. I can increase the timeout for those tests. I merged your code with master, will test soon.
🔥 🔥
❌ Add new shell asks for what to match against, but does not add match prop to .solidarity file
❌ Create rule without name but do not type name when prompted (accidental enter issue) result: creates name undefined
in json
❌ Add new requirement, but do not type a name (creates name undefined
in json)
❌ When adding a shell rule, it says "What is the name of the shell you'd like to add rule for", perhaps here it should say "What is the shell command you'd like to add a rule for"
Dude, epic PR! just a few issues to make it mergable. Also, do you want to do the docs in this PR or a separate one? I can help 👍
Whatever you think is best regarding the documentation! If you want to knock those out I can start on the issues you've raised.
I'm on staycation starting now so I should be able to knock these out pretty quickly.
Thanks for your review!
Looking good so far! 👍
Hey Matthew Thompson, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
@GantMan Barring the test passes on CI all the issues have been addressed.
Hey Matthew Thompson, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
Hey Matthew Thompson, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
@GantMan, Thoughts ^ seems to be happening more frequently after adding a few more tests.
Hey Matthew Thompson, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.
@mattathompson - looks like it might be timout.
We can adjust it like so: https://github.com/infinitered/solidarity/blob/d55a791bab899ef50f210f0895b430af87b30d5b/__tests__/integration/solidarity-check/check-invalid.ts#L11
Or jest.setTimeout(10000); // 10 second timeout
I re-ran tests. Looks good now! Also did a quick local test 👍
Let's do docs in a separate PR - Let me know if you want them or if I should.
NOICE! Love me some Jurassic Park.
On Sat, Dec 23, 2017 at 11:25 PM, Gant Laborde notifications@github.com wrote:
It's ALIVE!!!!
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/infinitered/solidarity/pull/134#issuecomment-353764535, or mute the thread https://github.com/notifications/unsubscribe-auth/AF83KwTdbiL_UgxsyOGO1lns2aOcLfJEks5tDdIngaJpZM4QqhGY .
Work in progress #75