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

Made script to convert directives to yml #4827

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Aryangp
Copy link

@Aryangp Aryangp commented Sep 10, 2023

📑 Summary

I have created a script that will convert the existing directive code to yml code as it scan throught the given file and change it to yml code

Resolves #4756

📏 Design Decisions

I have used simple fs library to scan througth the file and using regex pattern to find the directives in it when i find those pattern i make a function that take the json code and convert it into yml and that write that changed code in the existing file

📋 Tasks

Make sure you

@netlify
Copy link

netlify bot commented Sep 10, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 2774ca1
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/64fd64a4218de7000884e5a3
😎 Deploy Preview https://deploy-preview-4827--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

Merging #4827 (2774ca1) into develop (00d06c7) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4827      +/-   ##
===========================================
+ Coverage    79.52%   79.55%   +0.03%     
===========================================
  Files          147      147              
  Lines        13069    13069              
  Branches       613      613              
===========================================
+ Hits         10393    10397       +4     
+ Misses        2546     2542       -4     
  Partials       130      130              
Flag Coverage Δ
e2e 84.72% <ø> (+0.03%) ⬆️
unit 43.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

Copy link
Member

Choose a reason for hiding this comment

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

Please move the script to scripts.
And in the next major release we'll move it to .build since it should be a docs build step.

Copy link
Author

Choose a reason for hiding this comment

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

ok sure

Comment on lines +4 to +10
const filePath : string ="./test/test.md";

fs.readFile(filePath,'utf-8',(err,data)=>{
if(err){
console.log("Error reading file"+err);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Please wrap file reading in a function that should be executed when needed and filePath as a parameter.

Copy link
Author

Choose a reason for hiding this comment

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

ok i will do this change thanks

return;
}

const diresctiveRegix=/%%{init:(.*?)\}%%/g;
Copy link
Member

Choose a reason for hiding this comment

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

The regex should match everything between %%{ and }%%, if it's empty then just remove it.

Copy link
Author

Choose a reason for hiding this comment

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

ok but we have every directive with init in it so that is why i used this regex pattern

Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily, there are some other samples. It also can have empty spaces between after %%{, so do not rely on its content.

This one is escaped \}, but this one is not { (probably, should be).

const jsonContent = directiveContent.replace(/'/g, '"');

// Construct YAML content manually
const newYamlContent=`{"config":${jsonContent}}`;
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if used this instead:

config:
  pie: ...

Copy link
Member

Choose a reason for hiding this comment

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

And some data aren't inside `config. Please refer to docs for that.

Copy link
Author

Choose a reason for hiding this comment

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

can you elaborate this one as i am not able to understand the data part please

Copy link
Contributor

Choose a reason for hiding this comment

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

As the example I provided in the issue:

title: This is another complicated flow
config:
  theme: base
  sankey:
    nodeAlignment: justify
  flowchart:
    curve: cardinal
    htmlLabels: false

Not everything is under the config key

Copy link
Author

@Aryangp Aryangp Sep 12, 2023

Choose a reason for hiding this comment

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

hi @nirname can you help me with a regex patter that i can use to solve this above example and like can you give a directive example where above mentioned example is converted from

Copy link
Author

Choose a reason for hiding this comment

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

hi @Yokozuna59 can you provide me the link where data is not inside config and also the pie that you mentioned will come automatically can you help me out with it

Copy link
Contributor

Choose a reason for hiding this comment

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

@aloisklink we need your help. There is a schema which represents config, but it seems that the config is only a subset of yaml directives. Where to look, to have a full example?

@Aryangp
There is a schema which represents config. According to the docs we have config and title keys (at least)

Directives is what is being deprecated.

Addressing your question about syntax. These were recently removed from grammar, but you can get the idea
of what directives syntax was and what the current function to detect them is

There is an example of detect function call

This is how diagrams are preprocessed

Copy link
Member

Choose a reason for hiding this comment

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

@aloisklink we need your help. There is a schema which represents config, but it seems that the config is only a subset of yaml directives. Where to look, to have a full example?

So far, I don't think there isn't anything, see #4757. There's only the https://mermaid.js.org/config/configuration.html#frontmatter-config documentation, which isn't very substantial.

Contributions to improving the documentation are welcome (even I don't know what can and can't be put into frontmatter, e.g. does wrap: true do anything??)

Long-term, I'd love to make a JSON Schema that represents and documents the front matter, but I haven't had much time recently to work on mermaid unfortunately (and I first want to help improve the existing MermaidConfig JSON schema).

Copy link
Contributor

Choose a reason for hiding this comment

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

@Aryangp please keep title and config keys. I'm not sure what else should be here

Copy link
Contributor

@nirname nirname left a comment

Choose a reason for hiding this comment

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

Please, fix linting.
If this request only adds script, but does not update directives, then it should not resolve the issue

return;
}

const diresctiveRegix=/%%{init:(.*?)\}%%/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily, there are some other samples. It also can have empty spaces between after %%{, so do not rely on its content.

This one is escaped \}, but this one is not { (probably, should be).

const jsonContent = directiveContent.replace(/'/g, '"');

// Construct YAML content manually
const newYamlContent=`{"config":${jsonContent}}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

As the example I provided in the issue:

title: This is another complicated flow
config:
  theme: base
  sankey:
    nodeAlignment: justify
  flowchart:
    curve: cardinal
    htmlLabels: false

Not everything is under the config key

@sidharthv96
Copy link
Member

Please add the modified contents to this PR.
So we can validate the output and see if we miss anything.

@Yokozuna59
Copy link
Member

@Aryangp Is there any updates in this PR?

I'm currently working on the language server and I want to make it convert directives to front matter:

example.webm

@Aryangp
Copy link
Author

Aryangp commented Nov 3, 2023

@Yokozuna59 No sorry actually i had my university exams can you give me two days i will update the changes in the PR

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.

Remove usage of Directives in docs
5 participants