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

Jamr rebase #8

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Jamr rebase #8

wants to merge 11 commits into from

Conversation

peteoleary
Copy link

Lots of changes to allow JAMR to integrate with and run in a server environment.

@sammthomson
Copy link
Collaborator

Oh, I'm sorry, I was only looking at the last commit "Added --input and --output options...". Lot's more changes here than I thought...

var previousStage2: Map[String, GraphDecoder.Decoder] = Map()

// an optional callback that lets us get direct access to decoderResultGraph when parsing
var resultHandler: Option[Graph => Unit] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this ever being set to anything other than None.

Also, adding side-effecting callbacks makes the code harder to grok/change/debug/test. Is there a way to extract a method for the part of the code that produces decoderResultGraph instead?

@jflanigan
Copy link
Owner

Thanks Sam! Yeah I'll get to it soon, I just had some other stuff to take care of first.

@peteoleary
Copy link
Author

Jeff, any idea when you may take a look at this? I have more work I'd like to do but I'm holding off

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.

3 participants