Tidy up, modernise code and VS cleanup by BobLd · Pull Request #1920 · oxyplot/oxyplot

BobLd · 2022-08-16T21:46:54Z

Checklist

I have included examples or tests (no need)
I have updated the change log
I am listed in the CONTRIBUTORS file
I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

Tidy up, modernise code and VS cleanup (was previously in Tidy up, modernise code and improve render performance #1836)

@oxyplot/admins

VisualMelon · 2022-08-17T06:56:24Z

Can we lose the dropping of the space before /> (for this PR at least)? That's creating a lot of noise here, and wasn't in the original PR, so it's hard to check the other changes.

BobLd · 2022-08-17T08:14:25Z

@VisualMelon I know there are a lot of files changed due to that, my bad. But I can't really easilly revert that back on my side.

Maybe the easiest is to Hide whitespace in the 'Files changed' tab. This should help a lot, let me know what you think

VisualMelon · 2022-08-17T08:34:12Z

Maybe the easiest is to Hide whitespace in the 'Files changed' tab

Thanks ;) feels like I'm having to relearn how to use github at the moment.

Could we change it to the opposite? (i.e. add the spaces before /> rather than removing them). It's totally arbitrary, and if thsi was an accidental change on your behalf then that would be my preference if it's just as easy and no-one has any particular objection.

Tagging @HavenDV who has suggested we sort out some XAML standards, and who might know better what is the Zeitgeist on XML whitespace.

VisualMelon · 2022-08-17T08:35:55Z

Don't know why azure is failing at the moment; I doubt it's anything wrong with this PR.

VisualMelon · 2022-08-17T08:41:49Z

Slowly getting through these; all looks sane so far.

@BobLd just to check, are these otherwise all the automatic changes from the old PR?

VisualMelon

There are a few changes here I wouldn't personally make, but nothing too objectionable. is patterning, ?. on events, and readonly properties are nice. Everything else seems reasonable enough.

Couple suggestions below among examples of things that don't fill me with joy.

VisualMelon · 2022-08-17T08:44:37Z


                double nextInterval = goodIntervals.FirstOrDefault(i => i > interval);
-                if (nextInterval == default(double))
+                if (nextInterval == default)


Not sure I like this, but if it's automatic, so be it.

VisualMelon · 2022-08-17T08:47:43Z

        protected void BindCore(OxyInputGesture gesture, IViewCommand command)
        {
-            var current = this.InputCommandBindings.FirstOrDefault(icb => icb.Gesture.Equals(gesture));
+            var current = this.InputCommandBindings.Find(icb => icb.Gesture.Equals(gesture));


Prefer FirstOrDefault, but again, if this is an automatic change it's OK.

VisualMelon · 2022-08-17T08:53:40Z

                    else
                    {
-                        c = c >> 1;
+                        c >>= 1;


Not wild about this, but again, if it's automatic I'm not bothered.

VisualMelon · 2022-08-17T08:59:01Z

            if (dh > 0.5)
            {
-                dh -= 1.0;
+                dh--;


Again, prefer version which makes it clear this is a float, but not going to fight VS over it.

you're making a good point, happy to revert back and disable the warning. @VisualMelon what do you think?

Yep, if it's easy to disable this whole class of warning that'd be great.

VisualMelon · 2022-08-17T09:13:51Z

-            List<double> binBreaks = new List<double>(binCount + 1);
-
-            binBreaks.Add(start);
+            List<double> binBreaks = new List<double>(binCount + 1)


Don't like this much either, but again, if it's automatic, I'll live.

HavenDV · 2022-08-17T09:18:59Z

I would prefer this PR to be broken down into separate PRs that change one type of style at a time. For example, a PR that changes strings to nameof and so on. This makes it much easier to check.
Regarding spaces - https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/xmldoc/recommended-tags#generate-links-and-references contains values without spaces, so I would prefer that(in In my code, I also use xml tags in xml documentation without spaces before />).

VisualMelon · 2022-08-17T09:25:20Z

@HavenDV thanks. Looks like /> is OK then.

VisualMelon · 2022-08-17T09:31:17Z

Separate commits would be nice, but I don't think it's too big a problem if it's difficult or time-consuming.

HavenDV · 2022-08-17T10:34:02Z

I will wait for progress on this PR before continuing with nullable enable PRs

BobLd · 2022-08-20T17:17:59Z

@VisualMelon @HavenDV I've implemented your feedback.

Regarding the separate commits, I agree but it was really easier to do in one go
The changes here are indeed mainly from the other PR, but reviewed them again
For the Azure issue, I agree, I'm not sure what's going on... Let me know if you have an idea, or if I need to change something

Once I have your go, I will squash the commits to make it clean

VisualMelon · 2022-08-20T21:13:20Z

@BobLd I'll probably not have time to look at this properly for a quite a few days, but nothing jumps out to me as a problem. Re. azure, looks like it has never worked for the .NET Core versions of the tests, but at some point it started failing noisily rather than silently. I tried a few things to make it happy (see the open PRs), but failed and ran out of time. Not a problem for this (or any other) PR so long as we check it's the .NET Core tests failing and not something unexpected.

Don't worry about squashing: can do that within the github interface now.

VisualMelon

If it is possible to turn off the automatic contraction for x = x (whater) y to x (whatever)= y that would be good. None of the instance here improve readability, and I think a number of then make it worse.

I also really don't like all the checks of the form x?.y > z: this is producing a bool from an int? > int comparison and that's just wrong. bool? == bool is acceptable, but very happy for that to be disabled as well if they're all under one rule.

I'd also much prefer if it didn't remove all the empty lines within methods.

VisualMelon · 2022-08-21T06:22:57Z

                    double y_portion = delta_y / _y;
-                    lineend_x = y_portion * _x;
-                    lineend_y = y_portion * _y;
                    result = new ScreenPoint((y_portion * _x) + midPoint.X, (y_portion * _y) + midPoint.Y);


Maybe change this to use lineend_* instead; probably clearer.

VisualMelon · 2022-08-21T06:28:47Z

 // </copyright>
 // <summary>
-//   Provides information about an <see cref="OxyImage" />.
+//   Provides information about an <see cref="OxyImage />.


Something looks fishy here.

VisualMelon · 2022-08-21T06:38:21Z

                byte[] streamBytes = null;

-                if (this.contents != null && this.contents.Length > 0)
+                if (this.contents?.Length > 0)


I was a bit unsure about this last time, now I'm sure I don't like it... Don't want to lose the event refits etc. but these nullable comparisons are not nice. There are some instances of blah?.blah() == true also, but if they're not nearly as bad.

Don't know what others opinions are on this, but this sort of lifting makes me very uncomfortable.

VisualMelon · 2022-08-21T06:41:20Z

+                this.IsPanEnabled = (this.XAxis?.IsPanEnabled == true)
+                                    || (this.YAxis?.IsPanEnabled == true);

-                this.IsZoomEnabled = (this.XAxis != null && this.XAxis.IsZoomEnabled)
-                                     || (this.YAxis != null && this.YAxis.IsZoomEnabled);
+                this.IsZoomEnabled = (this.XAxis?.IsZoomEnabled == true)
+                                     || (this.YAxis?.IsZoomEnabled == true);


Per the above comment about lifting with comparisons, these are OK, but also not going to complain if they are lost if it means we can keep the rules simple.

VisualMelon · 2022-08-21T06:45:06Z

            if (dh > 0.5)
            {
-                dh -= 1.0;
+                dh--;


Yep, if it's easy to disable this whole class of warning that'd be great.

VisualMelon · 2022-08-21T07:00:18Z

            this.StrokeColor = OxyColors.Black;
            this.StrokeThickness = 0;
-            this.TrackerFormatString = XYAxisSeries.DefaultTrackerFormatString;
+            this.TrackerFormatString = DefaultTrackerFormatString;


These are OK, but again, if there's a way to turn them off, I think these are clearer as they were.

This could lead to inconsistency, however. What are others' opinions?

VisualMelon · 2022-08-21T07:07:54Z

                {
-                    this.MinY = Math.Min(this.ActualItems.Min(r => 0), this.ActualItems.Min(r => r.Height));
-                    this.MaxY = Math.Max(this.ActualItems.Max(r => 0), this.ActualItems.Max(r => r.Height));
+                    this.MinY = Math.Min(0, this.ActualItems.Min(r => r.Height));


(Note: this is OK: checks that the collection isn't empty above)

VisualMelon · 2022-08-21T07:24:32Z

@BobLd thanks for your changes. If we can change a few of the rules per the above, that's great. I don't know much about how this is all configured (must be editor config or something?), but obviously if we can use the VS defaults where possible and only disable those rules that we've agreed are problematic that'd be great. I didn't see a config file in the PR: does it just need adding or have I missed something? (e.g. my VS doesn't light up all of these same things).

Jonarw

Thanks for tackling this task! I think I can get behind pretty much all of the changes. I'd just have two additional comments:

(As already mentioned by @VisualMelon) Would it be possible to include a VS configuration file, so others would get the same automatic suggestions and refactorings regarding code style? I think this should be possible with an EditorConfig file.
Could we define (and enforce) a consistent code style regarding var usage? I think this is still a bit all over the place in different sections of code. My personal style is to always use var wherever possible, but I would also be happy with other conventions, as long as it is consistent.

VisualMelon · 2022-09-19T09:34:51Z

@BobLd CI should be happy after a rebase

VisualMelon · 2023-05-14T12:51:26Z

@BobLd if you can include a VS configuration file I'll take another look at this

Jonarw · 2023-07-12T05:56:20Z

Following the discussion in #2012, I will close this for now and revisit once we have fewer open PRs. If you have a VS config file that you could share you are welcome to do so, if not no worries :)

BobLd · 2023-07-12T16:31:29Z

Thanks for that @VisualMelon, I'll try to do something along the lines but focusing on other projects at the moment

Tidy up, modernise code and VS cleanup

e40a36b

BobLd force-pushed the cleanup branch from 7688b8a to e40a36b Compare August 16, 2022 21:48

VisualMelon reviewed Aug 17, 2022

View reviewed changes

BobLd added 2 commits August 20, 2022 17:50

Implement comments

33ed35f

Use ElementAtOrDefault instead of FirstOrDefault

2ddbd75

VisualMelon requested changes Aug 21, 2022

View reviewed changes

VisualMelon mentioned this pull request Aug 23, 2022

Fix CategoryAxis.ItemSource when not associated with a BarSeries #1912

Merged

4 tasks

Jonarw reviewed Aug 28, 2022

View reviewed changes

Jonarw closed this Jul 12, 2023

This was referenced Jul 20, 2023

Removed nesting levels in several places through block-scoped usings #2016

Closed

Consolidating Code Style #2017

Open

Uh oh!

Conversation

BobLd commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Changes proposed in this pull request:

Uh oh!

VisualMelon commented Aug 17, 2022

Uh oh!

BobLd commented Aug 17, 2022

Uh oh!

VisualMelon commented Aug 17, 2022

Uh oh!

VisualMelon commented Aug 17, 2022

Uh oh!

VisualMelon commented Aug 17, 2022

Uh oh!

VisualMelon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HavenDV commented Aug 17, 2022

Uh oh!

VisualMelon commented Aug 17, 2022

Uh oh!

VisualMelon commented Aug 17, 2022

Uh oh!

HavenDV commented Aug 17, 2022

Uh oh!

BobLd commented Aug 20, 2022

Uh oh!

VisualMelon commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VisualMelon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VisualMelon Aug 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VisualMelon commented Aug 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jonarw left a comment

Choose a reason for hiding this comment

Uh oh!

VisualMelon commented Sep 19, 2022

BobLd commented Aug 16, 2022 •

edited

Loading

VisualMelon left a comment •

edited

Loading

VisualMelon commented Aug 20, 2022 •

edited

Loading

VisualMelon Aug 21, 2022 •

edited

Loading

VisualMelon commented Aug 21, 2022 •

edited

Loading