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

Efficiency Improvments & Tests #4

Open
JFriel opened this issue Sep 1, 2018 · 6 comments
Open

Efficiency Improvments & Tests #4

JFriel opened this issue Sep 1, 2018 · 6 comments

Comments

@JFriel
Copy link

JFriel commented Sep 1, 2018

I really like this module! While it works well, would you be receptive to me putting in a PR with some efficiency improvements and test coverage?

I also think it could be expanded to include more of the standard braille language (https://www.ukaaf.org/wp-content/uploads/2015/05/Standard-English-Braille-PDF-file.pdf). But that is quite the undertaking, and should be its own issue

@Nonemoticoner
Copy link
Owner

I wonder if we could benefit in some of this code here: #1

And then possibly add some more improvements.

@JFriel
Copy link
Author

JFriel commented Sep 3, 2018

I think there is some good stuff in that PR, I can look at building on it and running some benchmarking on how the different primatives will perform before rolling out a large scale change

@Nonemoticoner
Copy link
Owner

AFAIK a dictionary will do faster than a single string holding everything as it is done currently. Having that confirmed would be nice though.

That PR is directed to master branch. So we probably need to make master our temporary playground for development if we wanna keep the @elenatorro's credit. That's not really a problem to me. I will write a simple note in README.md after merging. Unless there's some feature on GitHub I am not aware about that could lead to a single PR with multiple contributors (over different forks)?

@JFriel
Copy link
Author

JFriel commented Sep 4, 2018

I've made a PR for these changes.
Unfortunately I couldn't work out how to retain @elenatorro's work, so I think a mention in the README would be the best solutions. You were right about JS objects being better than string indexing. Some bench-marking showed objects were more efficient by a large margin.
PR Should wrap up this issue, so I'll close this issue after merge.

@Nonemoticoner
Copy link
Owner

Ah, I thought about different solution. I intended to merge #1 into master and upon that new master you could add some your improvements. This way everyone gets a proper credit.

Okay, so I just merged #6 because there was requirement for some fixes and improvements.

Now, upon new master branch we have, you can create your improvements and PR them.

@JFriel
Copy link
Author

JFriel commented Sep 5, 2018

Fixed PR to master to build on the new master

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

No branches or pull requests

2 participants