Skip to content
This repository has been archived by the owner on Apr 29, 2022. It is now read-only.

Implemented all special enums. #87

Merged
merged 14 commits into from
Sep 29, 2019
Merged

Implemented all special enums. #87

merged 14 commits into from
Sep 29, 2019

Conversation

SeppPenner
Copy link
Contributor

Fixes #9, Fixes #41.

@SeppPenner SeppPenner mentioned this pull request Sep 16, 2019
@SeppPenner
Copy link
Contributor Author

SeppPenner commented Sep 16, 2019

From #84 (comment):

  • There's a missing space in front of : in BorderCapStyle.
  • You should move the infos from the constructor summary in CubicInterpolationMode to the class summary.
  • "As per documentation here.." makes sense for the enums themself but doesn't add value if it's put in the property-summary.
  • The summaries of many properties need to be rewritten or adjusted. Sometimes there's a mention of 'center' but now we should reference Enum.Center' using a-tag. --> Which tag? <code></code>? Or do you mean using <see cref="Enum.Center"/>?
  • Since string enums can be null, I don't think we should set a default value and just let chart.js pick the appropriate one. Example of this is in PointLabels.FontStyle. Only use a default value if the default isn't null. --> Please check this, the default under https://www.chartjs.org/docs/latest/axes/labelling.html is 'normal', not null.

@SeppPenner
Copy link
Contributor Author

Fixes #85 as well.

@Joelius300
Copy link
Owner

Joelius300 commented Sep 16, 2019

Still a few things missing:

  • Missing space at PointStyle
  • Summary of FontStyle-property in PointLabels: You've now added one of those things that I want you to remove. Noone needs to know that this property stores CSS font-styles. They can already see that it stores a FontStyle and in the summary of FontStyle it says what it represents. You should add the stuff about css-font-styles there so it's seen everywhere FontStyle is used (it always represents css-font-styles not just in PointLabels). However I have to say that the stuff you removed from that property-summary was good, you just shouldn't have added that css stuff. You should remove it there and also in SubTicks, Ticks, PointLabels and probably many other places.
    This is what I meant by The summaries of many properties need to be rewritten or adjusted.. The description of what these properties represent can be moved to the enum-summary (in most cases).

Which tag? <code></code>? Or do you mean using <see cref="Enum.Center"/>?

Instead of writing When 'center' is set, blabla.. you should now write When <see cref="Enum.Center" /> is set, blabla... There are many places where this needs to be changed. These include (but there are probably many more):

  • CubicInterpolationMode
  • BorderAlign in PieDataset
  • BorderAlign in PolarAreaDataset

This is also part of what I meant by The summaries of many properties need to be rewritten or adjusted.

Since string enums can be null, I don't think we should set a default value and just let chart.js pick the appropriate one. Example of this is in PointLabels.FontStyle. Only use a default value if the default isn't null. --> Please check this, the default under https://www.chartjs.org/docs/latest/axes/labelling.html is 'normal', not null.

You missed my point. When we specify a value, anything other than null, the property will be set in js. If we set a null value (or nothing which implicitly sets null for reference types) the corresponding js-property will be undefined. If a js-property is undefined in a chart.js construct, the default option will be used by chart.js. This means that if we manually set FontStyle.Normal, it will always use that. If we set it to null, chart.js will see undefined and will use the default which is 'normal'.
To summarize: it will use 'normal' either way BUT if we set null we let chart.js choose the default which is not only more correct, it also shifts the responsibility of choosing the right default for that property to chart.js which is generally what we want considering chart.js is huge.

Also something we should continue doing is marking these enums as sealed. All the new ones need this and two we missed as well. The ones we missed are SteppedLine and AxisDisplay.

@SeppPenner
Copy link
Contributor Author

Should be all done.

@Joelius300
Copy link
Owner

I don't understand what's going on with CubicInterpolationModes summaries. 'default' went to <see cref="CubicInterpolationMode.Monotone" /> somehow. Also I made a stupid mistake when I said put it in the class summary. I meant the property summary of monotone. That's the one that actually makes sense :)

Otherwise it looks good 👍

@SeppPenner
Copy link
Contributor Author

Do you mean like this: 4613133?

@Joelius300
Copy link
Owner

Do you mean like this: 4613133?

Yes exactly :) I'll look over it in detail now and if I don't find any bigger issues, which I couldn't quickly resolve myself, I'll merge it soon (hopefully, I'm in class).

@Joelius300
Copy link
Owner

I have finished working on this and could merge this now. However I cannot push my commit mqttnettest:special-enums :/ I get a 403.. do you maybe need to add me as a colaborator @SeppPenner ?

@SeppPenner
Copy link
Contributor Author

Oh, sorry. Yes, you're added now.

@Joelius300 Joelius300 merged commit 74a8377 into Joelius300:master Sep 29, 2019
@Joelius300 Joelius300 deleted the special-enums branch September 29, 2019 16:05
@SeppPenner
Copy link
Contributor Author

Nice 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make BorderAlign a string enum Implement missing special enums
2 participants