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

Create workflow converting markdown-based changlog to lua-based changelog #6444

Merged
merged 22 commits into from
Oct 8, 2024

Conversation

Zjonn
Copy link
Contributor

@Zjonn Zjonn commented Sep 12, 2024

Resolves #6237 and #6238
Please take a look @Garanas

Description

Following PR is introducing python script converting Markdown files to Lua.
Markdown files are formatted before conversion to Lua file using mdformat (it's python alternative to prettier).

Why I chose python? There are several reasons:

  • It comes preinstalled in most Linux distibutions
  • Installed packages do not pollute the user environment (when using venv)
  • Has better linters than for example bash (shellcheck)

Additionaly I wrote setup_python_env.sh script for users not familiar with python, it can create python virtual enviorement for them, install required packages and source it.

Testing done on the proposed changes

Tested manually in local environment

Checklist

  • Changes are annotated, including comments where useful
  • Changes are documented in the changelog for the next game version

@Garanas
Copy link
Member

Garanas commented Sep 16, 2024

Hi @Zjonn, this looks exciting! I'll make some time next weekend to review it. Would you like me to incorporate it further into the workflow, or would you like to try and do that yourself?

@Zjonn
Copy link
Contributor Author

Zjonn commented Sep 16, 2024

Hi @Garanas
I will add this to the workflow myself (I hope 😄 ).

@BlackYps BlackYps marked this pull request as draft September 22, 2024 10:42
@BlackYps
Copy link
Contributor

Please set it to ready for review once you are done with your changes

@Zjonn Zjonn force-pushed the dd-6237-convert-md-to-lua branch 2 times, most recently from 7db1f24 to f2b33fa Compare September 22, 2024 22:33
@Zjonn Zjonn changed the title Create script converting markdown-based changlog to lua-based changelog Create workflow converting markdown-based changlog to lua-based changelog Sep 22, 2024
@Zjonn Zjonn marked this pull request as ready for review September 22, 2024 22:49
@Zjonn
Copy link
Contributor Author

Zjonn commented Sep 22, 2024

I added a workflow that can be run manually or through another workflow.
This has been tested in my fa fork, the results are as follows https://github.com/Zjonn/fa/actions/runs/10984939551

In short:

  • it takes all *.md files from changelog directory
  • runs markdown2lua.py on each of them
  • saves results in lua_changlog directory, which is later uploaded as an artifact

@Garanas
Copy link
Member

Garanas commented Sep 23, 2024

That looks like a great start! I've looked at a few files and so far it looks good. I'll come with a more in-depth review in a few days.

Copy link
Member

@Garanas Garanas left a comment

Choose a reason for hiding this comment

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

In general it looks good and the output is as promised!

A general suggestion:

  • (1) Generated files should be marked as generated. Please add a header block of comments stating that the lua changelog files are generated. Preferably with a reference to the source file and the logic responsible for generating it.

And two functional requests:

  • (1) The changelog files will be loaded as regular modules, via either the import statement or the doscript statement. For this the table inside the file needs to be named. Can you add a Changelog = {...} in front of each table?

  • (2) It would good if the integrity of the generated lua files are checked automatically. In a separate job you can install Lua, retrieve the artifact and check that all files can be interpret. The step should fail if one (or more) files can not be interpret. Make sure to use the needs field to tell the interpreter about the dependency between the jobs.

The last job is especially relevant since maintainers come and go. Not everyone may be aware of the details of this workflow file. And by testing the output automatically we can catch 'simple' issues long before it would reach any staging or production area.

For future reference, this was the output the review was made with:

It took a bit longer to review this than anticipated, life got in between 😃 .

scripts/markdown2lua.py Outdated Show resolved Hide resolved
scripts/requirements.txt Outdated Show resolved Hide resolved
scripts/setup_python_env.sh Outdated Show resolved Hide resolved
@Garanas Garanas added area: automated testing related to automated testing of Lua files area: changelog management related to (automated) management of changelogs area: documentation related to documentation to preserve knowledge and practices labels Oct 4, 2024
@Zjonn
Copy link
Contributor Author

Zjonn commented Oct 4, 2024

Thanks for the review @Garanas.
I'll temporarily mark PR as a draft version to satisfy @BlackYps ;)

I like the idea of adding information that the file was generated automatically and how

Can you add a Changelog = {...} in front of each table?

Sure, no problem

ld fail if one (or more) files can not be interpret. Make sure to use the needs field to tell the interpreter about the dependency between the jobs.

Thanks for the tip

@Zjonn Zjonn marked this pull request as draft October 4, 2024 18:01
scripts/markdown2lua.py Outdated Show resolved Hide resolved
@Zjonn Zjonn force-pushed the dd-6237-convert-md-to-lua branch 5 times, most recently from b555bc9 to ca0cd98 Compare October 7, 2024 23:05
@Zjonn
Copy link
Contributor Author

Zjonn commented Oct 7, 2024

I think I have made all the changes we have discussed. You can find newest workflow results here.

Validation job reuses already existing tests/run-syntax-test.sh, I made it executable (chmod +x).
I also moved all workflow bash scripts into bash subdirectory.

@Zjonn Zjonn marked this pull request as ready for review October 7, 2024 23:25
@Zjonn Zjonn requested a review from Garanas October 7, 2024 23:30
Copy link
Member

@Garanas Garanas left a comment

Choose a reason for hiding this comment

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

Excellent work 👍

@Garanas Garanas merged commit dc0f232 into FAForever:develop Oct 8, 2024
5 checks passed
@Garanas
Copy link
Member

Garanas commented Oct 8, 2024

As an extension to your work here: #6472 , with that we have everything we need to start updating the Lua of the game. We'll save several megabytes of memory all together and we'll have consistent changelogs 😃 !

@Garanas
Copy link
Member

Garanas commented Oct 8, 2024

@Zjonn I forgot - the contributions have no license at the top of the file. Would it be fine to license the contributions of this pull request with the MIT license?

@Zjonn
Copy link
Contributor Author

Zjonn commented Oct 10, 2024

@Garanas for some reason I don't see notifications when you are pinging me. MIT license sounds perfect!

About #6472, I can't wait to see the end result. Amount of deleted lines in there looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: automated testing related to automated testing of Lua files area: changelog management related to (automated) management of changelogs area: documentation related to documentation to preserve knowledge and practices
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert a markdown-based changelog file into a Lua-based changelog file
3 participants