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

pass through args to cover #72

Merged
merged 1 commit into from
Oct 12, 2016
Merged

Conversation

nathany
Copy link
Contributor

@nathany nathany commented Sep 9, 2016

like --include remote, but not including ExCoveralls specific args that cover won’t understand.

fixes #46

@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage increased (+0.1%) to 91.176% when pulling 817cbe4 on aai:pass_through_args into 1364ff7 on parroty:master.

@joshsmith
Copy link
Collaborator

This looks great. @parroty any chance this could be merged?

@joshsmith
Copy link
Collaborator

@parroty just pinging again on this. I would love to have this merged.

Copy link
Collaborator

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

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

Looks good; tests look good, too.

@parroty
Copy link
Owner

parroty commented Oct 11, 2016

I'm sorry about leaving the PR open for long time. It seems I made this PR in conflict status due to the merge of other PR.
Is it possible for you to update the conflict? (I tried but cannot push merge button).
I'll be pushing to hex after merging this commit.

@nathany
Copy link
Contributor Author

nathany commented Oct 11, 2016

Thanks @parroty. I'll get this updated today.

@joshsmith
Copy link
Collaborator

Thanks @parroty and @nathany! Let me know if you need a re-review after update.

like `—include remote`, but not including ExCoveralls specific args that cover won’t understand.

fixes parroty#46
Mix.Tasks.Coveralls.do_run(args,
[ type: "local",
detail: true,
filter: parsed[:filter] || [] ])
Copy link
Contributor Author

@nathany nathany Oct 11, 2016

Choose a reason for hiding this comment

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

the filter arg was appearing twice after the other changes, so I removed this.

not sure if the default of [] is necessary? may need to find a way to bring it back?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about that default either.

Copy link
Owner

Choose a reason for hiding this comment

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

Seems not necessary as it's checked at when used too.

{common_options, _remaining, _invalid} = OptionParser.parse(args, switches: switches, aliases: aliases)

# the switches that excoveralls supports
supported_switches = Enum.map(Keyword.keys(switches), fn(s) -> "--#{s}" end)
Copy link
Contributor Author

@nathany nathany Oct 11, 2016

Choose a reason for hiding this comment

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

equivalent to supported_switches = ~w(--filter --umbrella --verbose --pro --parallel -f -u -v) but this code adapts to new switches/aliases being added

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 92.089% when pulling 5585e04 on aai:pass_through_args into 14582d0 on parroty:master.

@nathany
Copy link
Contributor Author

nathany commented Oct 11, 2016

@joshsmith @parroty Please take another look. The requirements changed when adding the filter option (it takes a string, not a boolean). So this code is somewhat different than my previous version.

Copy link
Collaborator

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

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

I like what I see here, but would definitely like to see what @parroty thinks regarding the [] default.

Mix.Tasks.Coveralls.do_run(args,
[ type: "local",
detail: true,
filter: parsed[:filter] || [] ])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about that default either.

@parroty parroty merged commit 09b3931 into parroty:master Oct 12, 2016
@joshsmith
Copy link
Collaborator

🙌 thanks @nathany! All good now.

@parroty
Copy link
Owner

parroty commented Oct 12, 2016

Thanks @nathany @joshsmith !

@nathany nathany deleted the pass_through_args branch October 12, 2016 22:35
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.

--include tags
4 participants