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

Rename event args files to match the class name #286

Merged
merged 1 commit into from Jan 10, 2019

Conversation

Projects
None yet
3 participants
@hughbe
Copy link
Contributor

hughbe commented Jan 2, 2019

Rename the files, make properties autoprops

All tested 100% coverage by #274

@hughbe hughbe requested a review from dotnet/dotnet-winforms as a code owner Jan 2, 2019

@hughbe hughbe force-pushed the hughbe:cleanup-event-args branch 2 times, most recently from 558dd8e to 014358d Jan 2, 2019

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 2, 2019

@hughbe looks like there are a few conflicts to resolve. Once you've had a chance to look at those, I'll review the rest of these and hopefully we can get this in 😄

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 2, 2019

Could you provide some data backing up your 100% code coverage claim. What code coverage tool are you using?

@hughbe hughbe force-pushed the hughbe:cleanup-event-args branch from 014358d to 1ccd35c Jan 3, 2019

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 3, 2019

I'm using coverlet with the following test.sh script:

#!/bin/bash

set -e
dotnet test /Users/hugh/Documents/GitHub/winforms/src/System.Windows.Forms.Design/tests /P:CollectCoverage=true /p:CoverletOutputFormat=opencover /p:CoverletOutput=/Users/hugh/Documents/WinformsCoverage/coverage/coverage.xml /P:ExcludeByAttribute=ExcludeFromCodeCoverageAttribute
reportgenerator -reports:./coverage/coverage.xml -targetdir:./coverage

Of course, this is on mac, but the same thing works on Windows. I'm only using mac because writing these sort of unit tests are a pain on my windows vm

See https://github.com/hughbe/winforms/blob/c9f7e23a42cd9ac9e9180ff1b380875b9b08d87e/src/System.Windows.Forms.Design/tests/System.Windows.Forms.Design.Tests.csproj

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 3, 2019

I'm not especially happy about the scope of changes in commit 1ccd35c. I would have preferred to see each conceptual change as a separate commit. With the current form, it is impossible to state that the result is correct.

}

public Exception Exception { get; }

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

are these code paths equivalent?

using System;
namespace System.Windows.Forms {
/// <include file='doc\CacheVirtualItemsEventArgs.uex' path='docs/doc[@for="CacheVirtualItemsEventArgs"]/*' />
public class CacheVirtualItemsEventArgs : EventArgs {

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

Even though this may not be used in our codebase, this is a deletion of public API (see https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.cachevirtualitemseventargs?view=netframework-4.7.2) so deletion here may require api review. I will bring up to the team in order to get a definitive answer.

There are a couple of concerns here; on the top are

///
/// </para>
/// </devdoc>
public class ColumnClickEventArgs : EventArgs {

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

Same concerns as with CacheVirtualItemsEventArgs. This may be the case with any such public class deletions. I'll try to get a definitive answer here.

/// Provides data for the <see cref='System.Windows.Forms.ListView.OnColumnClick'/>
/// event.
/// </devdoc>
public class ColumnClickEventArgs : EventArgs

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

Was this class present in another file and you've put it in its own? Which file?

If not, why would you add it?

/// <include file='doc\ColumnWidthChangedEvent.uex' path='docs/doc[@for="ColumnWidthChangedEventArgs"]/*' />
/// <devdoc>
/// </devdoc>
public class ColumnWidthChangedEventArgs : EventArgs {

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

Same as CacheVirtualItemsEventArgs concern; marking for my own purposes


namespace System.Windows.Forms
{
public class ColumnWidthChangedEventArgs : EventArgs

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

Yeah did you add this one too? Sorry --there's a lot to look through here

using System.ComponentModel;

/// <include file='doc\ColumnWidthChangingEvent.uex' path='docs/doc[@for="ColumnWidthChangingEventArgs"]/*' />
public class ColumnWidthChangingEventArgs : CancelEventArgs {

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

Same as CacheVirtualItemsEventArgs concern; marking for my own purposes

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 3, 2019

Hey @hughbe, after looking through your changes here, I'm finding some equivalence classes of changes I'd like to break out and discuss separately:

  1. code cleanup --removing using statements, tabs, spaces, etc... these should be fine assuming everything builds etc...
  2. changing getters / setters --again, this should be fine so long as code paths persist
  3. changing variable names (capitalization) --this may be a problem if the classes are serialized or if these are public fields (see below point about new APIs)
  4. change variable types from specific to var --this is a functional change, of course, so we will need to discuss. The team has yet to settle on a consistent semantic here.
  5. removing "dead" public APIs--this will likely require more overhead, as I noted above
  6. adding new APIs/ events -- I first want to clarify with you if you have done this, or if any new classes are just the result of you copy / pasting those out of other code files. If you are going to add new public APIs, even for events, this is going to require API review.
/// </devdoc>
public void DrawBackground() {
if (Application.RenderWithVisualStyles) {
VisualStyleRenderer vsr = new VisualStyleRenderer(VisualStyleElement.Header.Item.Normal);

This comment has been minimized.

@zsd4yr

zsd4yr Jan 3, 2019

Member

we may have to discuss this change to var --unsure what the best strategy is here, and the team has yet to conclude a consistent semantic

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 3, 2019

By the way I have not deleted or added any public api - I think you’re reviewing all the changes rather than commit by commit. If you look at commit 1, then I’ve renamed all the files to match their actual name. Then the cleanup in commit 2 just demonstrates the cleanup. I’m guessing a limitation of github’s interface suggests that I’ve deleted and created new files

Will look into extracting changes

@hughbe hughbe force-pushed the hughbe:cleanup-event-args branch from 36a75ac to 1063344 Jan 3, 2019

@hughbe hughbe changed the title Cleanup event args Rename event args files to match the class name Jan 3, 2019

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 3, 2019

Updated the PR only to rename the files. Will submit the cleanup in a more appropriate and easy for in the next week

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 3, 2019

Updated the PR only to rename the files.

👍

@sharwell

This comment has been minimized.

Copy link
Member

sharwell commented Jan 3, 2019

📝 Once the entire solution conforms to this pattern, we can enabled the StyleCop Analyzers rule that checks for consistency during builds. This pull request only addresses one source, so the rule should remain disabled for now.

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 7, 2019

hey @hughbe unfortunately, even though we don't use this API, our API review board has determine that it is used by a sufficiently high percentage of user applications, which means we cannot in good faith remove it. Sorry you went to all the trouble.

This kind of thing may also occur elsewhere.

@zsd4yr zsd4yr closed this Jan 7, 2019

@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Jan 7, 2019

I’m very confused - this PR doesn’t change anything in the binary? This just literally changed the name of the source code file...

@zsd4yr zsd4yr reopened this Jan 7, 2019

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 7, 2019

Ah wait I see, that's right we moved that stuff didn't we... That's right you removed those changes... damn 1 sec...

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 7, 2019

good catch, sorry

@hughbe hughbe force-pushed the hughbe:cleanup-event-args branch from 1063344 to a48b28a Jan 7, 2019

@zsd4yr

zsd4yr approved these changes Jan 9, 2019

@zsd4yr zsd4yr merged commit 2639067 into dotnet:master Jan 10, 2019

1 check passed

license/cla All CLA requirements met.
Details

@hughbe hughbe deleted the hughbe:cleanup-event-args branch Jan 10, 2019

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