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

Update all hierarchy charts in the docs (re #451) #469

Merged
merged 2 commits into from Jun 22, 2017

Conversation

waveform80
Copy link
Member

Added notes on how the abstracts are represented, ensured all the class
hierarchies were up to date, and changed the orientation so the classes
are actually readable in the big chart.

@waveform80
Copy link
Member Author

Related to #451

@lurch
Copy link
Contributor

lurch commented Sep 26, 2016

ensured all the class hierarchies were up to date

LOL, I started working on exactly that myself yesterday! ;-)

I was wondering if addon-boards / HATS that connect to fixed physical pins on the Pi (i.e. that don't take a pin argument in their constructor) would be worth having a colour of their own?
And I can't remember the details off the top of my head (I'll try to double-check this evening) but there were some additional classes that I wondered if they should be coloured as 'abstract'.

Having got used to the charts being vertical, it seems strange now seeing them horizontal ;)

@codecov-io
Copy link

codecov-io commented Sep 26, 2016

Codecov Report

Merging #469 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #469   +/-   ##
=======================================
  Coverage   85.79%   85.79%           
=======================================
  Files          36       36           
  Lines        6541     6541           
=======================================
  Hits         5612     5612           
  Misses        929      929

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 412ca72...f8f810a. Read the comment docs.

@waveform80
Copy link
Member Author

LOL, I started working on exactly that myself yesterday! ;-)

Ah, sorry about that - we've probably got some conflicting changes then. I'm happy to close, merge-then-fix, or wait-and-rebase, whatever's easiest.

I was wondering if addon-boards / HATS that connect to fixed physical pins on the Pi (i.e. that don't take a pin argument in their constructor) would be worth having a colour of their own?

Hmm, that's a nice idea. Or just a different border style (I tend to be relatively conservative with colours in charts just in case I wander into colour-blind territory accidentally but we've only got two colours at the moment)

@lurch
Copy link
Contributor

lurch commented Sep 26, 2016

we've probably got some conflicting changes then

Nah, I didn't get round to finishing mine, so feel free to merge yours.
(I wrote a python script to scan all the .py files looking for class declarations - output attached)
hierarchy

we've only got two colours at the moment

three colours ;-P

@waveform80
Copy link
Member Author

(I wrote a python script to scan all the .py files looking for class declarations - output attached)

I was just thinking "it really is a pain maintaining these charts ... I ought to write a script to generate them". Bung in some filtering facilities so we can chop out horrid bits like GPIOBase, and grab segments of the hierarchy like "everything below OutputDevice, and the direct ancestors" so we can generate the smaller charts and that's exactly what I was looking for.

Any chance you want to PR it? Could just be a little python script under docs/ to run when we need to re-generate the charts.

I wouldn't add it into the Makefile just yet (we'd just wind up with chart regeneration in every bloody PR as each Python source change would cause the .dot files to regen which would rebuild the SVGs and so on). If I can figure out some way of telling it to regen the charts only if the hierarchy has actually changed, I'd do it but that can wait - it'd be great to just have the charts' source capable of being auto-generated.

@waveform80
Copy link
Member Author

three colours ;-P

Well ... blue, purple ... and a lighter shade of the blue. It's a shade alright, not a colour. That's my excuse, and I'm sticking to it. Ahem ;)

@lurch
Copy link
Contributor

lurch commented Sep 26, 2016

Bung in some filtering facilities so we can chop out horrid bits like GPIOBase, and grab segments of the hierarchy like "everything below OutputDevice, and the direct ancestors" so we can generate the smaller charts and that's exactly what I was looking for.

Yeah, that's pretty much what I was thinking, but hadn't got around to yet. I'll see if I can find some time to do some more work on it... (current version is attached, if you're curious)

update_class_hierarchy.py.txt

@waveform80
Copy link
Member Author

Oh, that's good stuff. I was actually thinking of importing it and working from the class meta-data but frankly regexes are easily "good enough" here and work nice and quickly. I'll see if I can refine this into something complete enough to generate the charts - thanks!

@lurch
Copy link
Contributor

lurch commented Sep 27, 2016

I'll see if I can refine this into something complete enough to generate the charts - thanks!

I've already started doing that myself, so I'd appreciate it if you didn't duplicate the effort... ;-)

@waveform80
Copy link
Member Author

I've already started doing that myself, so I'd appreciate it if you didn't duplicate the effort... ;-)

No worries! Look forward to seeing the result :)

@lurch
Copy link
Contributor

lurch commented Sep 28, 2016

Right, got my auto-charts-generation working, but still need to tidy up the code (it's a bit of a kludgy mess ATM). But it has led me to ponder some questions....

  1. Given that RGBLED and Energenie both descend directly from Device, why is Energenie in the https://gpiozero.readthedocs.io/en/v1.3.1/api_boards.html#base-classes diagram, but RGBLED in the https://gpiozero.readthedocs.io/en/v1.3.1/api_output.html#base-classes diagram?
  2. In https://gpiozero.readthedocs.io/en/v1.3.1/api_spi.html#base-classes SPIDevice is shown as an abstract class (light blue) but in https://gpiozero.readthedocs.io/en/v1.3.1/api_generic.html#generic-classes it's shown as a concrete class (dark blue)

there were some additional classes that I wondered if they should be coloured as 'abstract'.

I'm now thinking that perhaps InputDevice and OutputDevice should also be treated as abstract classes, since they're conceptually on the same level as InternalDevice and AnalogInputDevice?
However given that DigitalOutputDevice, PWMOutputDevice and DigitalInputDevice are currently shown as concrete classes, I wonder if SmoothedInputDevice should also be shown as concrete? (or should the former 3 be shown as abstract?)

Yes, I realise there are no hard'n'fast rules about which classes are 'concrete' and which are 'abstract'; but it might be nice to get a bit of consistency.

I also had some thoughts about the SPI classes, which I'll shortly be opening a separate PR for...

Obviously there's not really any way to auto-generate the composed_devices chart ( https://gpiozero.readthedocs.io/en/v1.3.1/api_boards.html#base-classes ), but I did have some thoughts there too...
To show that a TrafficLightsBuzzer is composed of a Button and a TrafficLights and a Buzzer, but a Motor is composed of PWMOutputDevices or DigitalOutputDevices, we could modify the chart like this : (by using dummy 'invisible' 0-size nodes)
composed_devices

And we could add labels to the edges, to show the number of sub-devices, but I dunno if that makes the chart too complicated :
composed_devices_with_labels

I also found that the full-hierarchy-of-everything diagram I attached in an earlier comment, is much more readable with the mixin classes not shown :-)
hierarchy_nomixins

@waveform80
Copy link
Member Author

I'm now thinking that perhaps InputDevice and OutputDevice should also be treated as abstract classes, since they're conceptually on the same level as InternalDevice and AnalogInputDevice?

Yup, sounds good to me.

However given that DigitalOutputDevice, PWMOutputDevice and DigitalInputDevice are currently shown as concrete classes, I wonder if SmoothedInputDevice should also be shown as concrete? (or should the former 3 be shown as abstract?)

Sounds reasonable - I think the only reason SmoothedInputDevice was marked abstract was because you "usually" wanted to override _read for it to make sense, but it's not mandatory (as demonstrated by MotionSensor). So, yeah, that could certainly be concrete.

To show that a TrafficLightsBuzzer is composed of a Button and a TrafficLights and a Buzzer, but a Motor is composed of PWMOutputDevices or DigitalOutputDevices, we could modify the chart like this : (by using dummy 'invisible' 0-size nodes)

Ooooh ... that's nice! Yes, definitely.

And we could add labels to the edges, to show the number of sub-devices, but I dunno if that makes the chart too complicated :

I could go for that - doesn't look overly crowded to me?

I also found that the full-hierarchy-of-everything diagram I attached in an earlier comment, is much more readable with the mixin classes not shown :-)

Yup, definitely better. Try rankdir=RL as well - tends to fit better on the relatively thin page-widths used by the RTD style.

@waveform80
Copy link
Member Author

Looking at the composition chart as well ... with the edge labels it might be clearer if the edges weren't dashed?

@lurch
Copy link
Contributor

lurch commented Sep 28, 2016

Glad to see we're in agreement :-)

Try rankdir=RL as well

Yeah, I was going to switch to rankdir=RL as part of my code-tidyup :)

Looking at the composition chart as well ... with the edge labels it might be clearer if the edges weren't dashed?

I didn't know if the lines were dashed because some kind of "class hierarchy conventions" said dashed lines implied composition? (which is why I left them as they were) shrug

What about my Energenie and RGBLED question? Does it make sense to keep them in different diagrams?

@waveform80
Copy link
Member Author

I didn't know if the lines were dashed because some kind of "class hierarchy conventions" said dashed lines implied composition? (which is why I left them as they were) shrug

No, no particular convention - just some vague attempt to make them obviously distinct from the rest of the charts, but the colour's probably enough for that (I don't go a bundle on the major diagrammatic systems - most are too complex for their own good).

What about my Energenie and RGBLED question? Does it make sense to keep them in different diagrams?

Good question. Why is Energenie in boards ... but RGBLED in outputs? I know I stuck RGBLED in outputs because, despite being a composite device under the covers, the user doesn't really see it as such - it's just a multi-coloured LED. Now ... why didn't I stick Energenie in there too? Erm ...

Yeah, I got nothing.

@bennuttall bennuttall added the RFC label Nov 28, 2016
@bennuttall bennuttall modified the milestone: v1.4 Dec 1, 2016
@bennuttall bennuttall mentioned this pull request Dec 1, 2016
@bennuttall bennuttall removed this from In progress in Roadmap Mar 28, 2017
@waveform80 waveform80 self-assigned this Jun 13, 2017
@waveform80
Copy link
Member Author

@lurch was there any progress on the auto-chart generator? Was just about to merge this and then re-read my way through all the comments :). Doesn't matter if it's still a kludgy mess - just the output would be fine for this PR and one of us can have a look at tidying the code up later. Don't worry if not - this can be merged as is anyway, and we can clean the rest up later.

@lurch
Copy link
Contributor

lurch commented Jun 14, 2017

@waveform80 good question. It's so long ago that I can't remember how far I got with it, but I'll see if I can dig it out over the weekend.

Added notes on how the abstracts are represented, ensured all the class
hierarchies were up to date, and changed the orientation so the classes
are actually readable in the big chart.
Heavily based on @lurch's efforts in gpiozero#469 but with some additional
filtering capabilities.
@waveform80 waveform80 merged commit 6e71f20 into gpiozero:master Jun 22, 2017
@waveform80 waveform80 deleted the doc-chart-updates branch June 22, 2017 13:01
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

Successfully merging this pull request may close these issues.

None yet

4 participants