Skip to content

Conversation

@robrap
Copy link
Contributor

@robrap robrap commented Apr 2, 2021

Description

Note to Reviewers: I suggest you move quickly over annotation deletions. We can rely on linting (current and future) to help ensure what I did was correct. In the meantime, it is cleaner and hopefully we'll have cleaner examples that are copy/pasted.

A variety of updates were made to improve the toggle documentation:

  • Added comments to help ensure that the waffle(), waffle_switches(),
    waffle_flags() anti-pattern won't be contagious (copied).
  • Some minor toggle_description updates.
  • Removed empty toggle_target_removal_date annotations for
    non-temporary toggles.
  • Removed empty optional toggle_warnings annotations.
  • Removed empty optional toggle_tickets annotations.
  • Removed deprecated toggle_category, toggle_status,
    and toggle_expiration_date annotations.
  • Fixed some indents, use cases, and implementations.

Supporting information

ARCHBOM-1721

A variety of updates were made to improve the toggle documentation:
* Added comments to help ensure that the waffle(), waffle_switches(),
  waffle_flags() anti-pattern won't be contagious (copied).
* Some minor toggle_description updates.
* Removed empty toggle_target_removal_date annotations for
  non-temporary toggles.
* Removed empty optional toggle_warnings annotations.
* Removed empty optional toggle_tickets annotations.
* Removed deprecated toggle_category, toggle_status,
  and toggle_expiration_date annotations.
* Fixed some indents, use cases, and implementations.

ARCHBOM-1721
# .. toggle_description: enables dashboard at /syadmin/ for django staff, for seeing overview of system status, for
# deleting and loading courses, for seeing log of git imports of courseware. Note that some views are noopen_edx
# .. toggle_use_cases: open_edx
# .. toggle_use_cases: temporary, open_edx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I believe toggle_use_cases is still confusing to people, which will result in loss of the most important data. Noting the toggles_warnings, it is clear that this is meant to be a temporary toggle, and knowing when a toggle is supposed to be temporary is probably the primary piece of data from this annotation. Note that toggle_target_removal_date is not a great indicator, because many people seem afraid to commit to any date.

This comment is to be used for a future discussion on the best annotation name and values to capture this info.

Comment on lines -88 to -89
# .. toggle_target_removal_date: None
# .. toggle_warnings: None
Copy link
Contributor Author

@robrap robrap Apr 2, 2021

Choose a reason for hiding this comment

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

Note: There are a lot of these issues, and it doesn't hurt anything necessarily, but makes the docs longer and makes it harder for the useful data to stand-out. Hopefully this clean-up will help some with the contagion factor, but linting is what would really nip this in the bud.

This comment is for future discussion.

# .. toggle_description: A "work-around" feature toggle meant to help in cases where some file uploads are not
# discoverable. If enabled, will iterate through all possible file key suffixes up to the max for displaying
# file metadata in staff assessments.
# file metadata in staff assessments.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Dropping the ability to add comments in-between toggle annotations with a group would enable linting to have caught this issue, because there were more toggles in the group. The same indent issue above wouldn't have been caught because it happened to also be the last annotation. The line with the bad-indent in both cases would have been lost until we improve this issue.

This comment is to help with future planning.


# .. toggle_name: videos.deprecate_youtube
# .. toggle_implementation: WaffleFlag
# .. toggle_implementation: CourseWaffleFlag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Linting may be able to catch some of these issues. There is at least one more case in this PR.

This comment is a note for future improvements.

@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

# .. toggle_description: Enables data new view for library on studio dashboard.
# .. toggle_use_cases: open_edx
# .. toggle_creation_date: 2020-07-8
# .. toggle_target_removal_date: None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewer Note: This PR should be removing toggle_target_removal_date: None in cases where toggle_use_cases does not include temporary. I think they are all the correct cases, but you don't need to spend a lot of time reviewing that to make sure it is perfect. It is an improvement that should help with the copy/paste contagion until we get some additional linting to give us what we want.

@robrap robrap merged commit 96be45f into master Apr 5, 2021
@robrap robrap deleted the robrap/ARCHBOM-1721-toggle-doc-cleanup branch April 5, 2021 14:47
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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