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

Outdated imports of variables from json file #2283

Closed
4 tasks
tlylt opened this issue Apr 16, 2023 · 11 comments · Fixed by #2301
Closed
4 tasks

Outdated imports of variables from json file #2283

tlylt opened this issue Apr 16, 2023 · 11 comments · Fixed by #2301
Assignees
Labels
c.Bug 🐛 d.easy p.Urgent To be done by next release, but follow normal release date

Comments

@tlylt
Copy link
Contributor

tlylt commented Apr 16, 2023

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

#2270, #1407

Tell us about your environment

Windows 10

MarkBind version

Master branch

Describe the bug and the steps to reproduce it

A warning is being logged when running markbind serve/build on a new site (that you can generate via markbind init)

markbind-cli: warn: You have a variable with no name! This variable will be ignored.

This is because in our template, we are still using the deprecated way of importing variables from external json files.

image

This way has been removed in #1407 and the proper way should be the stated one in https://markbind.org/userGuide/reusingContents.html#importing-variables-from-other-external-file-formats

Expected behavior

  • Remove the template's use of variable import
    • Check that no such warning when running markbind build on a newly generated site
  • Fix the functional test cases if needed
    • Check that no such warning when running npm run test

Anything else?

No response

@tlylt tlylt added c.Bug 🐛 p.Urgent To be done by next release, but follow normal release date d.easy labels Apr 16, 2023
@bibhu107
Copy link
Contributor

Hi @tlylt
Can I please take this task?

@tlylt
Copy link
Contributor Author

tlylt commented Apr 16, 2023

Hi @tlylt Can I please take this task?

Sure 👍. Note that this issue is slightly more involved, thus when in doubt please consult the dev guide https://markbind-master.netlify.app/devguide/devguide

@bibhu107
Copy link
Contributor

Hi @tlylt

Raised the PR #2290

@tlylt
Copy link
Contributor Author

tlylt commented Apr 17, 2023

Hi @ang-zeyu, do you still have the context for this variable feature?

  • "Importing variables from other external file formats" has been updated to use the "{% ext studentScoreboard = "userGuide/syntax/extra/scoreboard.json" %}" syntax
    • reflected in docs but not actually done in our template -> will proceed to fix that
  • Defining global variables via <variable name="year">2018</variable> syntax is in some cases have been updated to use the <span id="title" class="d-none">{{ title }}</span>.
    • is there a difference between page variable vs global variable?
    • page variable is not documented anymore
    • global variable is still mentioned in docs and still used in our docs -> need to fix both and fully deprecate it?

@ang-zeyu
Copy link
Contributor

reflected in docs but not actually done in our template -> will proceed to fix that

The global variables file does not support {% set %} nor {% ext %} syntax, something we could try to implement for syntax consistency. Only <variable name="...">...</variable> would work.

Defining global variables via 2018 syntax is in some cases have been updated to use the {{ title }}.

span id is an even older syntax than <variable> since MarkBind day 1.

is there a difference between page variable vs global variable?

Site variables are available for use anywhere and override everything (it's a very odd behaviour.. #1261).

page variable is not documented anymore

is this still in the docs somewhere? we should definitely remove if so.

global variable is still mentioned in docs and still used in our docs -> need to fix both and fully deprecate it?

not sure I understood this, having the idea of global variables is still very useful (e.g. in our docs). My only nit is the priority should be the opposite (why have something global override something more specific (nunjucks variables)? 👀)

@tlylt
Copy link
Contributor Author

tlylt commented Apr 28, 2023

Thanks @ang-zeyu for the comment. Sorry because there are a few things here...let me clarify further to ensure that I understand what you mean:

  • Page variable used to be a supported way to define a variable to be used just within a page. This was done via the <variable name="full_name">John Doe</variable> syntax within a .md file. However, we have supposedly deprecated and removed all such usage of page variables given that one can use {% set title = "Templates" %} to replace something like <variable name="title" id="title">Templates</variable>. The only legitimate use of <variable> within a page now is defining a variable within <include>
    • Questions:
      • Is the above accurate?
      • Does it mean that we do not want people to define page variables? From my brief search, it was only mentioned in passing that we support {% set %} syntax for defining variables within a page or when using dates.
      • There was a mention of the idea of page variable in https://markbind.org/userGuide/formattingContents.html#page-variables that seem like a left over.

As for global variable,

not sure I understood this, having the idea of global variables is still very useful (e.g. in our docs). My only nit is the priority should be the opposite (why have something global override something more specific (nunjucks variables)? 👀)

I wasn't proposing to remove it, just checking if it is still valid (since we are defining variables using the <variable> syntax in a .md file, it's like a page variable but it's ok here?), and there was a problem with importing the JSON files into the global variables in our default template.

  • Questions:
    • Is it ok that we have <variable> use in this way?
    • Any suggestions on what to do with the json import? <variable from syntax is not documented and not supposed to work right...

(also p.s. pinging you on slack in case you didn't get notified :))

@ang-zeyu
Copy link
Contributor

Is the above accurate?

Yes, except:

The only legitimate use of within a page now is defining a variable within

+ site/global variables

Does it mean that we do not want people to define page variables?

Yes, {% set %} served the same purpose and was far more powerful (scoping, types, ...).

From my brief search, it was only mentioned in passing that we support {% set %} syntax for defining variables within a page or when using dates.

This is a Nunjucks feature, so where relevant, we should stick to the term "Nunjucks variable" as much as possible for context.

I wasn't proposing to remove it, just checking if it is still valid

oh.. 👍

it's like a page variable

Is it ok that we have use in this way?

Not sure I understood this, I don't see it this way, the syntax is only a means to end.

We have 3 types of variables

  • anything native to nunjucks {% set/import/... %}
  • global + built-in variables
  • <include><variable>... overrides

I think, global variables were the first to adopt that syntax as well, then page variables which are now removed.

Imo, it is now inconsistent with {% set %} syntax overall and we could try to support that syntax in variables.md too. But that's for another topic..

problem with importing the JSON files

This is not supported inside it, so we could just remove those lines. (oversight in #1407)

@ang-zeyu
Copy link
Contributor

There was a mention of the idea of page variable in https://markbind.org/userGuide/formattingContents.html#page-variables that seem like a left over.

We should change this to "Nunjucks variables" and link there instead to be clear.

@tlylt
Copy link
Contributor Author

tlylt commented Apr 28, 2023

Got it thanks.

This is not supported inside it, so we could just remove those lines. (oversight in #1407)

Last qn, won't this be useful tho? So there's no way to globally import variables from a JSON file since we can't import it from variables.md?

@ang-zeyu
Copy link
Contributor

It could be, we can also think of how we want to achieve it with ⬇️

Imo, it is now inconsistent with {% set %} syntax overall and we could try to support that syntax in variables.md too. But that's for another topic

@tlylt
Copy link
Contributor Author

tlylt commented Apr 28, 2023

It could be, we can also think of how we want to achieve it with ⬇️

Imo, it is now inconsistent with {% set %} syntax overall and we could try to support that syntax in variables.md too. But that's for another topic

Okay sure will put up a separate issue for that 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug 🐛 d.easy p.Urgent To be done by next release, but follow normal release date
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants