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

Fix header flag #221

Merged
merged 1 commit into from
Jan 21, 2018
Merged

Fix header flag #221

merged 1 commit into from
Jan 21, 2018

Conversation

juanjoDiaz
Copy link
Collaborator

I realized of a couple things from my previous PR and am fixing it with the PR.

  • The noHeaders option worked as headers i.e. it set the headers when set to true. So makes sense to rename it.
  • The CLI flag --no-headers is a convention from commander. Any flag preceded by --no set's it's value to false if present, i.e. --no-headers actually sets headers = false

@coveralls
Copy link

coveralls commented Jan 20, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling ab96dc9 on juanjoDiaz:bugfix/fix_headers_option into 9c861ed on zemirco:master.

@knownasilya
Copy link
Collaborator

I think we want a header by default, so we'd want the noHeader behavior.

@juanjoDiaz
Copy link
Collaborator Author

The header is still by default. The only thing that's changed is the parameter name.

Before noHeader = true created header and noHeader = false not. That's the oposite of what I would expect.

With this PR header = true created header and header = false not. Which makes more sense to me.

Wdyt?

@knownasilya knownasilya merged commit 7f7338f into zemirco:master Jan 21, 2018
@knownasilya
Copy link
Collaborator

Ah yes, that makes sense.

@juanjoDiaz juanjoDiaz deleted the bugfix/fix_headers_option branch January 21, 2018 19:42
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