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

Clean up docs & tests for variables #2301

Merged
merged 3 commits into from
May 21, 2023
Merged

Conversation

tlylt
Copy link
Contributor

@tlylt tlylt commented Apr 28, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Fixes #2283

  • update docs to remove outdated mention of page variable
  • removed the incorrect and unsupported usage of variable imports from variables.md file
  • fix an outdated test case
  • add in variables.md file to remove unnecessary warning about missing file during functional tests

Anything you'd like to highlight/discuss:
Was expecting a need to update functional tests but doesn't seem to be required due to how variables.md and how nunjucks is processed before generating the HTML.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Clean up docs & tests for variables

Our docs on page variable is outdated due to feature removal. The
incorrect use of variable imports from json files within our variables.md
is problematic as it is no longer supported. A outdated test case still
exists for testing of page variable override. There are two warnings
of missing variables.md files in our functional test suite.

Keeping docs, test cases and implementation up-to-date is important.

Let's fix the docs, update the test cases, remove unnecessary warning
and ensure all variables.md do not contain the imports.

The above will fix the related issues around page variables and
variable imports.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Breaking change description:

A newly generated site (e.g. via markbind init) will no longer contain the following erroneous line in _markbind/variables.md:

<variable from="variables.json" />

This way of importing variables from an external file within the global variables.md is not supported.

For existing sites migrating to the latest version, please check that you do not have the above line in your _markbind/variables.md file as it will not work as expected and will log a warning. Please remove such a line if found.

Copy link
Contributor Author

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Added some comments for ease of reviewing

@@ -36,10 +36,10 @@ The default output format is `"ddd D MMM"` e.g. Fri 6 Mar
{{ njcode('base1 | date(format2, 0)') }} :glyphicon-arrow-right: {{ base1 | date(format2, 0) }}<br/>
{{ njcode('base1 | date(format2, 10)') }} :glyphicon-arrow-right: {{ base1 | date(format2, 10) }}<br/>

#### Page variables
Dates can be supplied using [page variables](../reusingContents.html#variables) for convenience.
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: page variables no longer supported via <variable> syntax, see #2283

@@ -9,6 +9,5 @@
<variable name="global_variable_overriding_included_variable">Global Variable Overriding Included Variable</variable>
<variable name="global_variable">Global Variable</variable>
<variable name="page_global_variable_overriding_page_variable">Global Variable Overriding Page Variable</variable>
<variable from="variable.json"></variable>
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: removed as this way of importing does not work in variables.md

@@ -71,6 +71,10 @@ Page Variable with {{ global_variable }}

**Test Page Variable and Included Variable Integrations**

{% set outerNunjucksVariable %}
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: adding this because in testPageVariablesInInclude.md below we will test if this outer variable is leaking into the inner page.

@@ -1,3 +1,3 @@
**Outer Page Variable Should Not Leak Into Inner Pages**

{{ nested_page_variable or "Outer Page Variable Should Not Leak Into Inner Pages" }}
{{ outerNunjucksVariable or "Outer Page Variable Should Not Leak Into Inner Pages" }}
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: updating this because it appears to be outdated. we used to have nested_page_variable defined in index.md but that has been deleted for a long time. See yucheng11122017@2ddf00e
image

@@ -0,0 +1,4 @@
<variable name="example">
To inject this HTML segment in your MarkBind files, use {{ example }} where you want to place it.
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: for some reason there's no variables.md in this and the other site above, but we should include it to avoid unnecessary warning.

@tlylt tlylt requested a review from jovyntls April 29, 2023 00:06
@tlylt tlylt requested a review from raysonkoh May 8, 2023 10:48
Copy link
Contributor

@ong6 ong6 left a comment

Choose a reason for hiding this comment

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

LGTM

@tlylt tlylt added this to the v5.0.0 milestone May 21, 2023
@tlylt tlylt merged commit 1f73886 into MarkBind:master May 21, 2023
@tlylt tlylt deleted the fix-variables branch May 21, 2023 08:57
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.

Outdated imports of variables from json file
2 participants