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

Status page fixes #2378

Merged
merged 5 commits into from Oct 18, 2020
Merged

Status page fixes #2378

merged 5 commits into from Oct 18, 2020

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Oct 16, 2020

  • Fixes Fleet States not... really... working on Ursula status page. #2368 (NO_FLEET_STATE_AVAILABLE and related issues). I was taking the current state info from the attributes set by update_snapshot(), while the most recent one is actually contained in .known_nodes.
  • Displays the correct state population.
  • Displays the total number of known nodes (as @cygnusv suggested).
  • Spaces out nickname symbols a little (thanks, @jMyles).
    Before:
    Screen Shot 2020-10-16 at 16 23 16
    After:
    Screen Shot 2020-10-16 at 16 22 34

Take this node's fleet state from `known_nodes` instead of the variables set in `update_snapshot`
@fjarri fjarri requested review from KPrasch and jMyles October 16, 2020 23:26
@fjarri fjarri added the Web Webpages label Oct 16, 2020
@@ -184,7 +196,7 @@ ${fleet_state_icon(state.checksum, state.nickname, len(state))}
</tr>
</table>

<h3>Known nodes</h3>
<h3>${len(known_nodes)} ${"known node" if len(known_nodes) == 1 else "known nodes"}:</h3>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is creeping in!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would look the same in Jinja, really. It's not business logic, it's display logic.

Copy link
Member

@KPrasch KPrasch Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point - in either case why not pre-compute this and pass it as rendering context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly why - it's not a context, it's display-specific. I could format it as

%if len(known_nodes)==1:
known node
%else:
known nodes
%endif

to avoid having Python logic in an evaluated expression, but it's only a formal change.

In general, if we start passing all the little things that only make sense inside the template from outside, we will end up with dozens of parameters in the call. For example, why not precalculate contrast_colors() outside, or checksum[0:8], or character_span()?

Copy link
Contributor Author

@fjarri fjarri Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add to that: in my understanding, a template takes data that is convenient to work with in the code (in this case, a node object) and turns it into something that a human can read (or a JSON parser, or a compiler, in other cases). Sometimes it means repeating information, working with grammar, adding colors and so on. All that is not the caller's business.

If we start passing everything to the template, we don't really need a template engine at all - f-strings would suffice. Then, since you'll need to precompute a lot of values, you'll extract that code into a function, or maybe even its own module, and call it by passing only the node object to it. Which means you'll again end up with a Mako template. Except Mako adds one small thing - it inverts the Python syntax in this module so the strings do not need to be escaped anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this reasoning.

Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤠 - Compelling points. Glad for the fixes too!

@KPrasch KPrasch merged commit 2abdbf1 into nucypher:main Oct 18, 2020
@fjarri fjarri deleted the status-page-fixes branch October 18, 2020 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Web Webpages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fleet States not... really... working on Ursula status page.
3 participants