Skip to content

Conversation

@bensalilijames
Copy link
Collaborator

@bensalilijames bensalilijames commented Oct 17, 2016

This PR aims to clean up a lot of SuperScript code, and now uses Babel to get the nice features of ES6 (+imports).

Description

  • Babel is used to compile ES6 => vanilla JS. No pun intended with the arrow. IMHO this is more readable and has helped me countless times in detecting bugs (some already existing in code) with variable scoping. Syntax is now more compact and followable.
  • The API exposed to the user is now just a SuperScript class. All you need to do in client code is:
bot = new SuperScript(options);
bot.importFile(file, (err) => {...});

I think this is nicer than creating your database, fact system and so on yourself - the bot will do all this for you. You can still configure the database name and fact system name if you so desire with the options passed into the constructor.

  • Unit tests are all now running and passing (with the exception of those that were being passed pre-cleanup).
  • The topicSystem is now called the chatSystem. I found it very confusing as a new user, since I expected it just to deal with topics, but it actually deals with everything. The DB interface now makes a bit more sense, with the models in a new 'models' folder, and helpers in the 'DB' folder.
  • Tried to enforce a consistent options-passing and naming structure. I found several cases of things being renamed as they passed through superscript - that should no longer be the case as it caused a lot of confusion for me. Generally, the things passed through are a system object, a user object, and any local options for that specific message.
  • You should now be able to run two SuperScript instances at a time (if you want!), since the global Mongoose dependency has been eliminated and replaced with instanced connections. I haven't tried this out yet though. I wanted to do this so I can run two (or more) bots on a single server that speak to each other.
  • Generally, everything runs a little faster compared to the original repo. Some of this may be down to the cleanups of qtypes and node-normalizer.
  • Linter is now an extension of Airbnb's linter, which is one of the most popular linters (if not THE most popular - 4 times as popular as 'standard'!). I liked its strictness, which encourages re-thinking about how you're writing code (e.g. not modifying parameters).

Motivation and Context

I've mainly outlined the motivation within the bullet points above. Hopefully this will be a big QoL improvement for SS maintenance in general.

How Has This Been Tested?

Unit tests that existed beforehand are all running. Trying to get npm packages linking locally was a pain, so I haven't been able to extensively check if client generation via the bin script works, however my hacky attempt did work, so fingers crossed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (I can do this when PR is close to being accepted) :)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Fix tests so they can run again

Clean up the message class

Clean up of various classes, start fixing issues

Fix message unit tests and various issues

Fix utils and wordnet unit tests

Fix continue unit tests

Fix a load more unit tests

Fix the remaining unit tests

Make superscript constructor pure and fix edge case of failing unit tests

Make message constructor pure

First linting pass

Second linting pass

Reshuffle of directory structure for babel

Lint clients and general cleanup

Last few tidy ups

Update dependencies and clean up tests

Update clients to new SS top level API

Get npm package ready

Fix es6 on travis
@coveralls
Copy link

coveralls commented Oct 31, 2016

Coverage Status

Changes Unknown when pulling 56ccde4 on benhjames:master into * on superscriptjs:master*.

@bensalilijames bensalilijames changed the base branch from master to v1.0.0 October 31, 2016 19:00
@bensalilijames bensalilijames merged commit 2824fff into superscriptjs:v1.0.0 Oct 31, 2016
duffrind pushed a commit to duffrind/superscript that referenced this pull request Aug 29, 2017
* ES6 commit (v1.0.0)

Fix tests so they can run again

Clean up the message class

Clean up of various classes, start fixing issues

Fix message unit tests and various issues

Fix utils and wordnet unit tests

Fix continue unit tests

Fix a load more unit tests

Fix the remaining unit tests

Make superscript constructor pure and fix edge case of failing unit tests

Make message constructor pure

First linting pass

Second linting pass

Reshuffle of directory structure for babel

Lint clients and general cleanup

Last few tidy ups

Update dependencies and clean up tests

Update clients to new SS top level API

Get npm package ready

Fix es6 on travis

* Diagnose Travis

* Fix Travis

* Fix ignore files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants