Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move getSourceString to String Utils #52

Merged
merged 1 commit into from
Sep 23, 2017
Merged

Move getSourceString to String Utils #52

merged 1 commit into from
Sep 23, 2017

Conversation

fitzn
Copy link
Contributor

@fitzn fitzn commented Sep 12, 2017

This commit is a non-functional change.

It moves the getSourceString function into its own string_utils module.
Additionally, a new string_utils test is added to check the functionality.

Lastly, it moves the test files into a new sub-directory named /test.

Tested: Locally

  • Ran the test file and verified new tests pass
  • Ran all tests in the /test directory and verified they all pass.

Example output

$ node node_modules/ava/cli.js test/string_utils.test.js 

  6 passed

and all tests:

$ node node_modules/ava/cli.js test/*.test.js

  18 passed

var STRING_JOIN_CHAR = StringUtils.STRING_JOIN_CHAR;
var getSourceString = StringUtils.getSourceString;

function runTest(name, expectation, func) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will dropped once #48 is merged and the repo has access to yarn+ava.

* getSourceString
*/

var simpleString = 'simple test string';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you care more about the data type than the data itself, it might be good to use generators for this if/when #42 goes in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up leaving this as-is for now, but happy to go with the string gen instead, I just couldn't get it on the (quick) first go around.

string_utils.js Outdated

const STRING_JOIN_CHAR = "\x00";

function getSourceString(obj) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get a docblock on this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - good idea.

@fitzn
Copy link
Contributor Author

fitzn commented Sep 12, 2017

@thedavidmeister Updated with docblock.

@tonyofbyteball
Copy link
Member

could we move tests to a subfolder? This flat nonstructure was already too big even before their arrival.

@thedavidmeister
Copy link
Contributor

@tonyofbyteball lol, i specifically made the test files flat to mimic the existing structure.

Would you be able to document how you want this repo to be structured to make it easier for us to follow?

This commit is a non-functional change.

It moves the getSourceString function into its own string_utils module.
Additionally, a new string_utils test is added to check the functionality.

Lastly, it moves the test files into a new sub-directory named test.

**Tested: Locally**
- Ran all tests in the /test directory and verified they pass.
@fitzn
Copy link
Contributor Author

fitzn commented Sep 23, 2017

@tonyofbyteball @thedavidmeister I put both existing tests into a new /test sub-directory. Let me know if this is not what you have in mind.

@fitzn
Copy link
Contributor Author

fitzn commented Sep 23, 2017

Rebased onto #46 to use ava for tests.

@tonyofbyteball
Copy link
Member

Would you be able to document how you want this repo to be structured to make it easier for us to follow?

moving tests to a subdir is already an improvement. I don't see how the rest could be organized significantly better, creating subdirs for a 1 or 2 of files wouldn't make much sense.

@tonyofbyteball tonyofbyteball merged commit d003286 into byteball:master Sep 23, 2017
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.

None yet

3 participants