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

Refactor cube summary #3952

Closed

Conversation

stephenworsley
Copy link
Contributor

🚀 Pull Request

Description

Refactors cube summary to make tabular representation more straightforward.

@trexfeathers
Copy link
Contributor

trexfeathers commented Jan 21, 2021

This is a big thing to throw into v3.0.x, which I believe needs to be cut in the next couple of days. Are you sure this is the right place to point the PR?
(I had been planning to point my PR at the ugrid branch, although I haven't consulted anyone on that)

@pp-mo
Copy link
Member

pp-mo commented Jan 21, 2021

big thing to throw into v3.0.x

It was my suggestion to target v3, thinking we might otherwise fall foul of forthcoming code changes there, as it's moved on so much. Master will hopefully soon move forward to encompass v3.
Until then, this is only a draft, so not intending to merge it directly / immediately.

We can re-consider this, should not be a big problem as this is only one commit + introduces all-new content.

@pp-mo
Copy link
Member

pp-mo commented Jan 21, 2021

The link check is general problem, not introduced here of course : @rcomer latest #3951 is also failing on this.

@rcomer
Copy link
Member

rcomer commented Jan 21, 2021

link check is a general problem

I notice that @bjlittle is methodically turning them off (e.g. here).

@pp-mo
Copy link
Member

pp-mo commented Jan 22, 2021

I just put up a PR with some ideas that I've been adopting to get the basic operations working.
That's just to ensure we have a common agenda really : I guess you're already working on some similar changes, which may not look the same (in which case, let's have yours + I'll review)

@pp-mo
Copy link
Member

pp-mo commented Jan 22, 2021

Before making more concrete suggestions/requests, I want to wait until the code here settles a bit + you have some added tests.

In the meantime though, I do have some general points you might like to consider ...

(1) the instance properties (either controls or maybe just "constants") intended to control indenting : item_indent, section_indent, extra_indent ...

  • are all a bit broken at this point, because they are not "passed down" when creating the contained Section-s (and Summary-s within them)
  • I think as far as possible we should be considering those things as 'styling' and so maybe should be leaving all that to the representation routines, not encoding them as properties of the basic summary object. Tellingly (I think!), such choices may not be the same for different representations...
  • ... However, as discussed yesterday, I can see that there is an apparent need to control the 'clip length' in some areas : I think that can best be managed as a simple 'max length' value, maybe doesn't even really need to be different between 'item' and 'extra'. Actually, maybe such necessary clipping can also be delegated to the representation methods?

(2) Where we have general context and control-info to "pass down" through Section and Summary creation, I think it would be easier + better to simply pass the parent reference to the sub-components.

  • For example, instead of passing 'cube' in VectorSection(title, cube, contents, iscoord) and then VectorSummary(cube, vector, iscoord), it would be simpler to pass the "summary" object itself as a constructor arg (i.e. make VectorSection(self, title, contents, iscoord)) and then refer to self.summary.cube in the lower-level code. This then works for any context or options settings which are set at the top level, e.g. could be used for the 'indent' controls at present (though I'm now arguing for removing those anyway!)

@pp-mo pp-mo mentioned this pull request Feb 5, 2021
@stephenworsley stephenworsley changed the base branch from v3.0.x to mesh-data-model February 5, 2021 17:09
@stephenworsley stephenworsley changed the base branch from mesh-data-model to v3.0.x February 5, 2021 17:10
@pp-mo pp-mo mentioned this pull request Feb 8, 2021
@bjlittle
Copy link
Member

bjlittle commented Feb 8, 2021

@stephenworsley and @pp-mo I think this PR shouldn't be targeting v3.0.x, unless I misunderstand the intention here... but things have moved on rapidly since this PR was originally raised.

Is it okay to close this PR?

I think that we should be targeting master now, which will eventually end up in v3.1.x.

@stephenworsley
Copy link
Contributor Author

@bjlittle this can be closed since it has been superceded by #3987.

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.

5 participants