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

first commit #1

Merged
merged 6 commits into from
Nov 3, 2014
Merged

first commit #1

merged 6 commits into from
Nov 3, 2014

Conversation

azylman
Copy link
Contributor

@azylman azylman commented Oct 31, 2014

I thought about getting rid of the test CSVs and instead using buffer data in the test file, but this ended up being convenient for running the command line tool against, as well.

@azylman azylman force-pushed the birthday branch 5 times, most recently from cf97ab4 to f7d9ed7 Compare October 31, 2014 23:18
@schimmy
Copy link
Contributor

schimmy commented Nov 1, 2014

The main thing I'm thinking about is stderr / stdout and exit codes - I think all the stuff being output right now should be output via stderr. I could imagine this program printing out the invalid lines to stdout in the future, for instance. I could see this (and the 'is valid') still printing to stdout, perhaps:

for _, invalid := range invalids {
    fmt.Println(invalid.Error())
}

Also, we should exit with non-zero (and non-1 to distinguish from the other error cases) when the csv fails validation - that way scripting this will be easier.

I'm a little concerned that there are some other tools for this - specifically, we could use something which has a csv schema potentially, and really tighten up what we accept.

@azylman
Copy link
Contributor Author

azylman commented Nov 1, 2014

Why stderr? The point of the program is to tell you whether or not a CSV is valid, and why, so that information doesn't seem like "debug" output, it seems like expected output of the program.

Agree about this, though: "we should exit with non-zero (and non-1 to distinguish from the other error cases) when the csv fails validation - that way scripting this will be easier"

I looked into alternatives before I started and there are two of them, one written in JS and one written in Ruby.

The one written in JS doesn't have any documentation and no one uses it (no watchers on Github).

The one in Ruby looks pretty sweet (and supports schemas like you mentioned!), but it's not a command line utility, isn't standalone (so would be harder to distribute to XA, for instance), and adds a lot of complexity for features that we don't need right now.

@rgarcia
Copy link
Member

rgarcia commented Nov 2, 2014

Tread carefully here--it looks like there are some strong opinions on both sides about how linters should treat exit codes. E.g. golint: golang/lint#66 https://github.com/golang/lint/search?q=Exit&type=Issues&utf8=%E2%9C%93

@schimmy
Copy link
Contributor

schimmy commented Nov 3, 2014

I suppose I am thinking that stdout should usually be easily parseable by machines, and stderr is usually for humans. I'm not 100% tied to this, though, and the current output format is not too hard to regex against if you were trying to incorporate this into a pipeline/workflow.

And you've convinced me on the alternatives :). Definitely being able to direct people to a binary is crucial!

@azylman
Copy link
Contributor Author

azylman commented Nov 3, 2014

stdout should usually be easily parseable by machines, and stderr is usually for humans

Is that a common convention? I'd always considered it as "stdout is expected output of a program, stderr is for any errors, warnings, or logs that got you there" - basically that the command-line program is a function and the arguments are stdin and the return values are stdout. That's why I have this going to stdout currently, and I like it that way, but I'd be willing to change it if this is against convention.

re: exit codes, I haven't read the golint issues yet, but my initial feeling is to make it exit code 0 when the file is valid (as it currently is), exit code 1 when it's unable to parse the file (as it currently is), and exit code 2 when it was able to parse the file but some lines couldn't lint (currently exit code 0).

@schimmy
Copy link
Contributor

schimmy commented Nov 3, 2014

Love the exit code idea. For stdout / stderr, this is not a really strong piece of evidence, but http://unix.stackexchange.com/questions/79315/when-to-use-redirection-to-stderr-in-shell-scripts#comment128689_79321
I particularly like the idea of "Imagine a pipe" when thinking about this stuff...

Again, as long as it's consistent and easily parseable by machines, I'm not too worried. I think I crossed us over into bikeshedding at some point on this one :-/

build/$(EXECUTABLE)-v$(VERSION)-linux-amd64:
GOARCH=amd64 GOOS=linux go build -o "$@/$(EXECUTABLE)" $(PKG)/cmd/csvlint
build/$(EXECUTABLE)-v$(VERSION)-windows-amd64:
GOARCH=amd64 GOOS=windows go build -o "$@/$(EXECUTABLE).exe" $(PKG)/cmd/csvlint
Copy link
Contributor

Choose a reason for hiding this comment

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

this is going to be so critical, love it

@schimmy
Copy link
Contributor

schimmy commented Nov 3, 2014

LGTM otherwise!

@schimmy schimmy assigned azylman and unassigned schimmy Nov 3, 2014
azylman added a commit that referenced this pull request Nov 3, 2014
@azylman azylman merged commit be8a7b0 into master Nov 3, 2014
@azylman azylman deleted the birthday branch November 3, 2014 18:55
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