Skip to content

Cleanup#39

Merged
emosbaugh merged 7 commits intomasterfrom
cleanup
Nov 7, 2017
Merged

Cleanup#39
emosbaugh merged 7 commits intomasterfrom
cleanup

Conversation

@emosbaugh
Copy link
Copy Markdown
Member

No description provided.

Comment thread pkg/plans/byte-source.go Outdated
err := errors.New("no data source defined for task")
if raw {
results = append(results, &types.Result{Description: task.RawPath})
results = append(results, &types.Result{Path: task.RawPath})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Defining path will cause these to appear in index.json, but there won't be any files at the path.

Comment thread pkg/plans/stream-source.go Outdated
results := []*types.Result{}

descriptionResults := []*types.Result{}
pathResults := []*types.Result{}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why do we need results and pathResults? If the spec asked for a file then there should be a result for it. If there's an error the result will be added to error.json and if there's a path it will be added to index.json.

Copy link
Copy Markdown
Member Author

@emosbaugh emosbaugh Nov 7, 2017

Choose a reason for hiding this comment

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

It would be nice to be able to tell what path the error refers to. can you think of a better way to do this?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The path could go in the description

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It might make sense to put all the specs in index.json and add results and errors as fields to that spec so that it's clear which files and errors came from each spec.

Comment thread pkg/types/plugin.go

type BytesScrubber func([]byte) []byte

func BytesProducerFromStreamProducer(ss StreamProducer) BytesProducer {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not use plans.StreamSource when you have a stream producer?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

because plans.StreamSource doesnt have a parser

Copy link
Copy Markdown

@areed areed left a comment

Choose a reason for hiding this comment

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

It looks cleaner but I left a couple questions

@emosbaugh emosbaugh merged commit 5f90670 into master Nov 7, 2017
@emosbaugh emosbaugh deleted the cleanup branch November 7, 2017 20:24
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