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

Revert "Add TakesFile indicator to flag" #858

Closed
wants to merge 1 commit into from

Conversation

AudriusButkevicius
Copy link
Contributor

Reverts #851

Suggest we rethink this before this spreads too far.

@AudriusButkevicius AudriusButkevicius requested a review from a team as a code owner August 12, 2019 20:31
@codecov
Copy link

codecov bot commented Aug 12, 2019

Codecov Report

Merging #858 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #858   +/-   ##
=======================================
  Coverage   71.33%   71.33%           
=======================================
  Files          18       18           
  Lines        2362     2362           
=======================================
  Hits         1685     1685           
  Misses        572      572           
  Partials      105      105
Impacted Files Coverage Δ
flag_generated.go 60.69% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cc7e98...7f972ee. Read the comment docs.

@coilysiren
Copy link
Member

@AudriusButkevicius can you give a more thorough explanation of why you're opening this PR? I don't have enough context to make a decision here.

@coilysiren
Copy link
Member

coilysiren commented Aug 13, 2019

If we want to revert this, I think the reversion needs to come with a thorough explanation in the PR description. I don't understand why would should be reverting this, which means I am very likely to unknowingly approve similar changes like this in the future. If there's some API stability north star we are working towards, then it would help me out if that north star was described. My current state is that I have no idea why someone would / would not add a certain feature, and I need to be educated on this if the goal is for me to make decisions that move the API to a good place.

@asahasrabuddhe
Copy link
Member

Let's first finalize the discussion on #851 and then come to a conclusion about whether or not this PR is even necessary

@AudriusButkevicius
Copy link
Contributor Author

AudriusButkevicius commented Aug 13, 2019

The rationale is on the original PR that this is reverting.

@coilysiren
Copy link
Member

I consider this PR blocked pending more discussion, similarly to #860

@coilysiren coilysiren added the status/blocked requires some external dependency label Aug 23, 2019
@AudriusButkevicius
Copy link
Contributor Author

We can probably close this and merge #860 before we release anything.

@coilysiren
Copy link
Member

We can probably close this and merge #860 before we release anything.

👍

@coilysiren coilysiren closed this Aug 24, 2019
@AudriusButkevicius AudriusButkevicius deleted the revert-851-takes-file branch August 24, 2019 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/blocked requires some external dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants