search

tdmalone / working-plusplus

Like plusplus.chat, but one that actually works, because you can host it yourself 😉
MIT License
23 stars 41 forks source link

Implement rich messages #1

Closed SkUrRiEr closed 6 years ago

SkUrRiEr commented 6 years ago

This is preliminary work towards being able to specify messages as

Awesome, <item> has <score> point<plural> now! or *<score>* Ha ha ha! (lightning cracks)

The next step is to convert the message store.

SkUrRiEr commented 6 years ago

This is hilariously untested BTW.

tdmalone commented 6 years ago

Looks good! I've retargeted this PR for the dev branch as there has been a bit of work on restructuring (mainly to make it easier to test) that hasn't been merged into master yet. dev also now has a .travis.yml too, so Travis CI should automatically lint and test the code in PRs (well, as much as the tests that have been written so far anyway!) when there's no conflicts.

If you want to test it locally against Slack, there's also more instructions now in the dev README on running locally.

tdmalone commented 6 years ago

I've fixed the conflicts between this PR and dev, and pushed them to the branch rich-messages.

The new diff can be seen here: https://github.com/tdmalone/working-plusplus/compare/dev...rich-messages

I don't think I can push to this PR; @SkUrRiEr if you'd like to select 'allow edits from maintainers' on the right then I can do that, otherwise feel free to bring the changes into your branch yourself.

Now that dev is in there, Travis is building - the current result can be seen here. We'll just need to adjust the tests on the operations to utilise the new constants, and also run a yarn fix to resolve most of the linting issues; there should then only be a few to resolve manually.

I don't think I put this in the eslintrc - I probably should - but it would be good if you could also change the vars to lets if they are gonna change, or const if they're not gonna change (see this article for a quick backgrounder).

tdmalone commented 6 years ago

Oh - one other thing - I went for operations and operation when merging in https://github.com/tdmalone/working-plusplus/compare/dev...rich-messages, rather than operation and op. I feel it's a bit clearer.

SkUrRiEr commented 6 years ago

Ok, I've added Yarn as a dev dependency as I don't have it installed globally and it's required to run tests.

I've also fixed a couple of bugs and removed some broken tests.

Hopefully it's mergeable now!

tdmalone commented 6 years ago

Ok, I've added Yarn as a dev dependency as I don't have it installed globally and it's required to run tests.

Oh, good point. It's not actually required but I should document that. You can alternatively just use npm run test, the only thing with npm is you won't get the exact same dependencies but that's not likely to be a massive issue.

Tests are passing but linting is failing - I can fix that up after merge.

Have you tested against a real Slack?

SkUrRiEr commented 6 years ago

I can't (easily) test against a "real" Slack, so I'm very much relying on the tests.

tdmalone commented 6 years ago

Ok no worries =] I will merge now and test locally before I merge into master!

(Automated tests aren't anywhere near good enough yet, but hopefully not too far off.. just need to finish messing around with Express and then mock Slack)