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

Add Contract.Requires(!String.IsNullOrEmpty(arg)) action #49

Open
denwanke opened this issue Nov 13, 2014 · 5 comments
Open

Add Contract.Requires(!String.IsNullOrEmpty(arg)) action #49

denwanke opened this issue Nov 13, 2014 · 5 comments

Comments

@denwanke
Copy link

Please provide the above menu action (which would correspond to the Code Contracts 'crsn' snippet).

@SergeyTeplyakov
Copy link
Owner

Didn't get this.

Quick fix mean the fix for some code issue. Or you mean conversion from Contract.Requires(arg != null) to Contract.Requires(!string.IsNullOrEmpty(arg))?

@denwanke
Copy link
Author

I mean the action in the R# menu on the left side (Alt+Enter), when you point on a method parameter. As for now, there is only one option available from the extension: "Requires 'arg' is not null", that produces Contract.Requires(arg != null);. This corresponds to the CC 'crn' snippet. So we have a choice what to use - the Resharper action or the code snippet. But for string parameters there is one more pattern to go (besides comparing to null) - checking that a string value is neither null nor empty by using Contract.Requires(!String.IsNullOrEmpty(arg)), which I believe is 99% of all use cases. Code Contracts is aware of this pattern and offers a 'crsn' snippet for this case. But not the extension. That's the point. We just need one more option to chose from in the R# menu. Comparing to null for strings should remain, however (for that very rare 1% of all cases).

@denwanke denwanke changed the title Add Contract.Requires(!String.IsNullOrEmpty(arg)) quick-fix Add Contract.Requires(!String.IsNullOrEmpty(arg)) action Nov 14, 2014
@SergeyTeplyakov
Copy link
Owner

Sorry, your comments slightly confused me:

  1. I know what the "Alt+Enter" means, but not sure you do;). Alt + Enter shows two types of actions: quick action or quick fix. Quick action is available based on the source code and quick fixes are available only as a fixes for some code issue. You mentioned quick fix initially, but this is not a quick fixes.
  2. Another wrong assumption is related to snippets. I don't use any snippets like crn or crsn and this is another assumption that confuses me. The code was generated by the R# API without any crsn or crn.
  3. There is a built-in functionality in the tool that you're looking for. You should just enable it.
    image

I thought about adding to different context action, and, maybe this is a better option. But still not sure about that.

@denwanke
Copy link
Author

  1. I really confused "quick action" with "quick fix". Sorry about that. Have changed the title accordingly.
  2. I know the CC snippets have nothing to do with the R# or the extension. I mentioned them just to point out, that with the snippets one can do more than with the R#/Extension - this is not good. Since we want the extension to be AT LEAST as powerful as the Microsoft snippets, don't we? :)
  3. I wasn't aware of the setting, that was the problem. My bad. But even with the setting I still don't feel comfortable. Because it gives me either null or null/empty check and I'd like to have BOTH. Maybe with the ability to choose the default one (as with the first option "Use Contract.Requires<...> by default"). I would even suggest to have this option (let's call it "prefer checking for empty strings") set by default as the extension is first installed (to reflect the mainstream usage).
    Moreover, the current behavior is really confusing: I now have the option checked and still see "Check 'prm' for null" in the quick action menu. Although it produces the correct results (!String.IsNullOrEmpty(arg)), I would never conclude it from the action's title, because what I want to do is to check for an empty string, not for a null reference.

What would be really cool:

  • if there is no contract check for a given string parameter yet, BOTH actions are offered (for null and null+empty checks). Moreover, the both of them with all possible variations (just like now, with/without ArgumentException, in sub-menus)
  • if one of the forms of contract checks is already present, a quick action is offered to convert the existing check to the other variant. I.e. (arg!=null) --> !String.IsNullOrEmpty(arg) and vice versa. And as you mentioned above, a "quick fix" could be even a better way to go. Vut if it's not possible, we still need the second remaining action (would have to delete the old check manually, then).

What also would be nice is to have a code inspection based on the preference chosen in the extension options. Background: if the user means he mostly wants to check for empty strings (instead of null references), the inspection would highlight the opposite usages. It shouldn't be very obtrusive, however ("suggestion" would suffice).

@SergeyTeplyakov
Copy link
Owner

I'll additional context action and will use default action based on the configuration settings in the upcoming release.

And will create additional task for your remaining suggestions. They're more complex to implement so I'll add them in one of the upcoming versions.

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

No branches or pull requests

2 participants