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

Merge future to features/patterns #9302

Merged
merged 20 commits into from
Feb 29, 2016

Conversation

jaredpar
Copy link
Member

I'm running tests locally. Once they complete I'm going to merge and start a PR back into future.

huizhonglong and others added 20 commits February 23, 2016 14:38
Implement a fix all provider for `Prefer Implicit / Explicit type` code
style. We simply use the default batchfixer here to fix all nodes which
don't comply with the user settings.

Notes:

1. we do not differentiate between Notification styles when fixing all
non-compliant nodes. i.e., say 2 different locations violate the user
preference, but one has notification level 'warn', while the other has
notification level 'info', invoking fix-all here will fix both the
nodes.

In other words, fix-all doesn't differentiate between two nodes that
have different notification levels or severity levels.

2. this is a given, but still noting it down. we do differentiate
between nodes with different style preferences. For e.g: say the user
prefers explicit type for built-in types but implicit type everywhere
else, the batch fixer would fix only nodes that do not comply with the
preferences.

3. the current implementation fixes all occurrences of the same kind of
violation - using explicit type when implicit is preferred or
vice-versa, however, it does not fix all violations between kinds
(implicit -> explicit, explicit -> implicit). This is because my current
implementation has a different diagnostic for each kind of violation.
Having a single diagnostic and fixer would have been better for this.
I'll add this to my todo list.
The user option gets style preference for built-in types but the
implementation checked only for built-in types used _with_ literals on
the right hand side. I guess I had two different choices in mind - for
built in types overall, or where we use literals in the right hand side
and implemented half of both.

Anyway, this clears that up - the option gets the preference for
built-in types and the implementation checks for predefined types (not
literals alone).
These tests were testing that replacement to var works where type is
apparent. but they also had another rule that said prefer built-in
types. so i update these tests to not use built-in types and replace
them with their framework counterparts. That way we're now clearly
testing if the fix works where type is apparent.
…port-for-squiggles

Get support for squiggles and LB in "AnyCode" workspace
Due to issues dotnet#7584 and dotnet#7587 adding `this.`/`Me.` is disabled for
methods and events.

Also add tests for verifying that no diagnostics are generated for static
access or `base`/`MyBase` access.
If the preference is set to "use explicit type for built-in type" and if
the code is non-compliant (uses var), then we need to flag this case and
offer a fix.

Basically, this adds to my definition of IsInIntrinsicContext. It is now
either that the declaration has a predefined type or the declaration
type is inferred and the inferred type is an intrinsic type.

For the purposes of this feature, we also consider string and object
to be built-in types, in addition to the actual intrinsic types provided
by the compiler.
Use var codestyle updates

Part of dotnet#9155

Change summary:
Implements a fix all provider for the codestyle, and adds corresponding tests.
Fixes an oversight with previous implementation of detecting built-in types.
@jaredpar
Copy link
Member Author

CC @gafter, @AlekseyTs, @CyrusNajmabadi

jaredpar added a commit that referenced this pull request Feb 29, 2016
Merge future to features/patterns
@jaredpar jaredpar merged commit 701c3ae into dotnet:features/patterns Feb 29, 2016
@jaredpar jaredpar deleted the patterns-merge branch February 29, 2016 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants