Skip to content

Conversation

@estute
Copy link
Contributor

@estute estute commented Jun 7, 2019

Description: This adds anchors to each admonition describing an individual code annotation group. This way, an external report, like the one generated by querying the database for feature toggle state, can link back to the annotation report for the toggles definition.

JIRA: https://openedx.atlassian.net/browse/TE-2875

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

{% if annotation.report_group_id %}
{% if loop.changed(annotation.report_group_id) %}

{% if annotation.annotation_token == '.. toggle_name:' %}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's been a while since I've been in here, but I'm not sure this line is necessary? If so, we should not be hard coding the annotation token, since any given configuration may not have this token in it. Is the goal to have an anchor for the first annotation in each group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. I was only thinking of my needs here. Yes, that is the goal. I see what you are saying about it not being necessary if we use the loop.changed part. I suppose I wanted to the anchor to be on something recognizable, in this case, the name of the toggle, because I am trying to have bidirectional links between this report and another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmedx I changed it to be more generic. Can you take another look when you get a chance?

Copy link
Contributor

@bmedx bmedx left a comment

Choose a reason for hiding this comment

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

lgtm 👍 assuming you've tried it on a bit data set and it looked ok

@estute estute force-pushed the estute/add-anchors-to-annotation-doc branch 2 times, most recently from 5b286b2 to a85c0df Compare June 19, 2019 15:05
@estute estute force-pushed the estute/add-anchors-to-annotation-doc branch from 079cb81 to 00d9fda Compare June 21, 2019 14:40
@estute estute merged commit 7d1a2f4 into master Jun 21, 2019
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.

3 participants