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

More Tests, Unified Input Parser, Modularization #28

Merged
merged 3 commits into from
Jun 27, 2017

Conversation

ccpandhare
Copy link
Collaborator

I have added more tests in this Pull Request.
My work on the Unified Input isn't over so it can't be merged in.

@ccpandhare ccpandhare changed the title More Complete Tests More Tests, Unified Input Parser Jun 25, 2017
@ccpandhare
Copy link
Collaborator Author

I could complete the Unified Input Parser today itself. So pushed that in too.
It reduces file size and input is parsed in a more central manner.

@ccpandhare ccpandhare changed the title More Tests, Unified Input Parser More Tests, Unified Input Parser, Modularization Jun 26, 2017
@ccpandhare
Copy link
Collaborator Author

I have broken down ImageSequencer.js into various Modules as suggested in #26.

I have a small problem here @jywarren. That is of nomenclature. I am really confused as to how to name these new files which have been generated.
For instance,
The function addSteps now lives in src/AddSteps.js
Should the file be called AddSteps.js or Module.AddSteps.js or something else?
What is the convention here?

Also, Added the method loadImages to the Unified Input Parser.

Also, updated the syntax for the loadImages method to allow callbacks for JSON Inputs as well and updated the Readme.md regarding the same.

@jywarren
Copy link
Member

It's a matter of style. In https://github.com/publiclab/PublicLab.Editor, I tended towards longer names, but you can also use directories to group and organize, so for example:

/src/modules/addSteps.js

I think that's nice and succinct, and as long as you respect the same convention everywhere, that's fine.

Is this ready? I'd like, as soon as possible, to be working in smaller, self-contained PRs so that it's easier for me (and others) to evaluate the one thing each PR intends to do, look at its implementation and the test which demonstrates that function, and merge it in. I know you're in a "bootstrapping phase" right now so this is OK, but the sooner we merge this in and start working in smaller pieces, the better. Is this ready to merge?

@jywarren
Copy link
Member

If it's ready to go, please merge it in! And we can start on the next round of changes.

@ccpandhare
Copy link
Collaborator Author

Okay!

@ccpandhare ccpandhare merged commit 9e9d0af into publiclab:master Jun 27, 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.

2 participants