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

Remove Obsolete Attributes #2219

Closed
rprouse opened this Issue Jun 4, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@rprouse
Member

rprouse commented Jun 4, 2017

We obsoleted several attributes like FixtureSetup with the 3.0 release. It is probably about time we removed them.

Other candidates for removal are some of the fluent syntax for constraints where we came up with better naming.

@stevenaw

This comment has been minimized.

Show comment
Hide comment
@stevenaw

stevenaw Jun 11, 2017

Contributor

I can take this on.
Can I please be assigned to it?

Contributor

stevenaw commented Jun 11, 2017

I can take this on.
Can I please be assigned to it?

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jun 11, 2017

Member

Hi @stevenaw Sure thing. I sent you an invitation as a contributor. After you accept I'll be able to make assignments to you.

Member

CharliePoole commented Jun 11, 2017

Hi @stevenaw Sure thing. I sent you an invitation as a contributor. After you accept I'll be able to make assignments to you.

@stevenaw

This comment has been minimized.

Show comment
Hide comment
@stevenaw

stevenaw Jun 12, 2017

Contributor

Thanks @CharliePoole , I've accepted the invite 🙂

Contributor

stevenaw commented Jun 12, 2017

Thanks @CharliePoole , I've accepted the invite 🙂

@Lette

This comment has been minimized.

Show comment
Hide comment
@Lette

Lette Jun 12, 2017

Contributor

Just a thought:

If we're following SemVer, we have to consider that removing these attributes might be a breaking change, which will require a bump of the major version. So, this is probably something that shouldn't be done before the 4.0 release.

Contributor

Lette commented Jun 12, 2017

Just a thought:

If we're following SemVer, we have to consider that removing these attributes might be a breaking change, which will require a bump of the major version. So, this is probably something that shouldn't be done before the 4.0 release.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jun 12, 2017

Member

That's a point. Not sure how much into semver everyone is at this point though. @rprouse ?

Member

CharliePoole commented Jun 12, 2017

That's a point. Not sure how much into semver everyone is at this point though. @rprouse ?

@NN---

This comment has been minimized.

Show comment
Hide comment
@NN---

NN--- Jun 26, 2017

Please do it in 4.0.
It will break my code and many other projects too.

NN--- commented Jun 26, 2017

Please do it in 4.0.
It will break my code and many other projects too.

@CharliePoole

This comment has been minimized.

Show comment
Hide comment
@CharliePoole

CharliePoole Jun 26, 2017

Member

@rprouse How do you want to handle this? Should we have a 4.0 Milestone?

Member

CharliePoole commented Jun 26, 2017

@rprouse How do you want to handle this? Should we have a 4.0 Milestone?

@rprouse

This comment has been minimized.

Show comment
Hide comment
@rprouse

rprouse Jun 27, 2017

Member

If we follow SemVer to the letter, then we probably should hold off on this to a 4.0 release. I think you could argue though that since most of the attributes were obsoleted in 3.0 and there have been many releases to fix the warnings it isn't a breaking change.

AFAIK, the only obsolete feature that isn't a simple search and replace update is the AssertHelper. AssertHelper has only been obsolete for one release, so removing it may be pre-mature. Updating everything else should only take a few minutes, even for large solutions. @nn or @Lette is there anything other than AssertHelper that you see as a major or difficult change?

Personally, I am surprised that people haven't already updated their code, but maybe I just hate warnings more than other people 😄 I am also surprised that people continue to use AssertHelper since it is so limited compared to the other asserts.

Member

rprouse commented Jun 27, 2017

If we follow SemVer to the letter, then we probably should hold off on this to a 4.0 release. I think you could argue though that since most of the attributes were obsoleted in 3.0 and there have been many releases to fix the warnings it isn't a breaking change.

AFAIK, the only obsolete feature that isn't a simple search and replace update is the AssertHelper. AssertHelper has only been obsolete for one release, so removing it may be pre-mature. Updating everything else should only take a few minutes, even for large solutions. @nn or @Lette is there anything other than AssertHelper that you see as a major or difficult change?

Personally, I am surprised that people haven't already updated their code, but maybe I just hate warnings more than other people 😄 I am also surprised that people continue to use AssertHelper since it is so limited compared to the other asserts.

@stevenaw

This comment has been minimized.

Show comment
Hide comment
@stevenaw

stevenaw Jun 27, 2017

Contributor
Contributor

stevenaw commented Jun 27, 2017

@NN---

This comment has been minimized.

Show comment
Hide comment
@NN---

NN--- Jun 27, 2017

I have some obsolete attributes. Not really much work to find and replace.

The main difficulty was ExpectedException attribute removal in 3.0 which I can say was a very good idea.
Too bad it existed from the beginning..

NN--- commented Jun 27, 2017

I have some obsolete attributes. Not really much work to find and replace.

The main difficulty was ExpectedException attribute removal in 3.0 which I can say was a very good idea.
Too bad it existed from the beginning..

@rprouse

This comment has been minimized.

Show comment
Hide comment
@rprouse

rprouse Jun 27, 2017

Member

@stevenaw if you want to do a PR, it might help us visualize what is changing and what the impact will be without searching through the code ourselves. If you do the PR, please put a do not merge in the description and you should probably hold off on doing more work until we make a final decision on this issue. @Lette and @NN--- have raised good points and I think we should evaluate the impact of this change.

Member

rprouse commented Jun 27, 2017

@stevenaw if you want to do a PR, it might help us visualize what is changing and what the impact will be without searching through the code ourselves. If you do the PR, please put a do not merge in the description and you should probably hold off on doing more work until we make a final decision on this issue. @Lette and @NN--- have raised good points and I think we should evaluate the impact of this change.

@stevenaw

This comment has been minimized.

Show comment
Hide comment
@stevenaw

stevenaw Jul 29, 2017

Contributor

@rprouse @CharliePoole I've put up a sample PR here: #2338
It doesn't look like I'm able to update labels, so I've put a big "DO NOT MERGE" heading in the description

Contributor

stevenaw commented Jul 29, 2017

@rprouse @CharliePoole I've put up a sample PR here: #2338
It doesn't look like I'm able to update labels, so I've put a big "DO NOT MERGE" heading in the description

@stevenaw

This comment has been minimized.

Show comment
Hide comment
@stevenaw

stevenaw Aug 2, 2017

Contributor

Sorry, I forgot to structure my GH comment so that this would close automatically.
I'm unable to manually close it. @rprouse @CharliePoole @ChrisMaddock can you guys help out?

I see we're handling obsoleted non-attributes (constraints) in a different issue (#2342)

Contributor

stevenaw commented Aug 2, 2017

Sorry, I forgot to structure my GH comment so that this would close automatically.
I'm unable to manually close it. @rprouse @CharliePoole @ChrisMaddock can you guys help out?

I see we're handling obsoleted non-attributes (constraints) in a different issue (#2342)

@rprouse rprouse added this to the 3.8 milestone Aug 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment