Skip to content

Conversation

jamesb93
Copy link
Member

@jamesb93 jamesb93 commented Feb 8, 2022

This enables you to define:

max-seealso: fzero~
pd-related: sigmund~

in the YAML docs which is then rendered in the appropriate templates.

I've opted for a relatively permissive try/Except block to catch cases where these new things are not defined or just not needed. It thusly throws a warning if they aren't which given the current unfinished state of the docs is a bit noisy.

On another note does this block up the YAML to RST milestone?

@jamesb93 jamesb93 requested review from weefuzzy and tedmoore February 8, 2022 21:59
@jamesb93 jamesb93 self-assigned this Feb 8, 2022
@jamesb93 jamesb93 added the enhancement New feature or request label Feb 8, 2022
@jamesb93 jamesb93 added this to the YAML to RST milestone Feb 8, 2022
{%- endfor -%}

{%-for s in pd_seealso -%}
<h3 class="seealso_name"><a href="{{ s }}.html">{{ s }}</a></h3>
Copy link
Member

Choose a reason for hiding this comment

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

maybe strip the <a> – presumably it's not linking anywhere useful if it's for generic PD objects

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah is the HTML doc a FluCoMa specific thing? In the case, indeed bye bye html

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

data['seealso'] = [x for x in tidy_split(data.pop('see-also',''))]


try:
Copy link
Member

Choose a reason for hiding this comment

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

This seems like overkill. Does it need a warning. Wouldn't

data['max_seealso`] = data.get('max_seealso`,'').split(',') 

do the trick, without the need for try..except?

Copy link
Member

Choose a reason for hiding this comment

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

give or take some curious use of backticks instead of single quotes

Copy link
Member Author

Choose a reason for hiding this comment

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

Have to say I prefer my overkill as its obvious what happens in either scenario. Happy to budge if you think its better to go your way though

Copy link
Member Author

Choose a reason for hiding this comment

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

Just going from my own experience parsing the transformer code for the first time I found the many nested dict.pop() sort of stuff quite windy to get my head around but thats just my 2c.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay this is much more simplified now :)

@jamesb93 jamesb93 removed this from the YAML to RST milestone Feb 9, 2022
Copy link
Member

@tedmoore tedmoore left a comment

Choose a reason for hiding this comment

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

This is great. My only reservation is that now we have

  • see-also which is FluCoMa related
  • max-seealso
  • pd-seealso
  • sc-related -- which is actually in SC called "see also", so maybe we need to change sc-related to sc-seealso

and I wouldnt be opposed to changing see-also to flucoma-seealso.

I think this would make more clear to anyone coming into the RST what these are for and which are parallel to each other.

@jamesb93
Copy link
Member Author

jamesb93 commented Feb 9, 2022

The naming being consistent would make it obvious how all those bits talk to each other, I agree. What does @weefuzzy think of that?

@jamesb93
Copy link
Member Author

Ths is also made slightly more complicated by the YAMl to RST conversion so I think it best to put it into a separate PR

@jamesb93 jamesb93 merged commit e878615 into flucoma:dev Feb 10, 2022
@jamesb93 jamesb93 deleted the enhance/cce-related branch June 15, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants