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

Add note regarding writable PVs and msg_apply_changes #57

Merged
merged 4 commits into from
Aug 23, 2024

Conversation

xkaraman
Copy link
Contributor

Add a note about unintended side-effects when applying multiple changes all once and suggest applying them each one.

@miconda
Copy link
Member

miconda commented Aug 19, 2024

Regarding the format fix, the ToC was generated by a tool that says it has GitHub markdown profile support. Not sure what the lint action is using as markdown reference, it has to be clarified and maybe tuned to accept some variants, otherwise it is useless to rely on markdown helper tools if afterwards everything has to be edited manually.

The link to ToC inside ToC is not really necessary, being more or less at the same line.

The note is not clear in my opinion, there are many variables that are writable and not affecting msg_apply_changes(), I guess you meant the variables related to From/To header attributes, maybe they should be listed there, with the reference also to uac_replace_to()/uac_replace_from(), which are actually recommended to be used instead.

@xkaraman
Copy link
Contributor Author

Using pre-commit run --all-files produces a long list of errors not only in main.md but other files as well like cookbooks/3.2.x up to cookbooks/5.8.x, features/, install/, and cookbooks/devel.

pre-commit was introduced 6 months ago maybe without applying it to all files and the tool that generated ToC is not up to date or following the supposed conventions? Whatever the case, I could just not push the format fix, since I only fixed it because I was referencing it in the note.

The note was in general to be honest, since it was not only for From/To attributes but also applied for some others like the issue I referenced to (kamailio/kamailio#1719) and I thought it's more of a general case.

Regarding the uac functions I can of course update the respected PV to actually recommend those instead.

@miconda
Copy link
Member

miconda commented Aug 20, 2024

The current markdown format of the wiki content is mostly what was generated by the dokuwiki-to-markdown conversion tool. The conversion was not perfect, I did a lot of manual updates afterwards, but I couldn't cover everything, it is a lot to do to sanitize everything.

The tool seems ok, because GitHub markdown says that - * + chars can be used for list items:

But the lint action might not support it or needs to be configured accordingly for the GitHub flavour.

Regarding the variable, $ru, $rU, .. and others are changing the sip message, but they use a different approach. The issue you references is about functions that change the SIP message, not variables. In general, besides the R-URI related variables, only functions should change the SIP message. At some point, for simple/basic needs, a few variables related to To/From headers were made writable, but the uac functions should be recommended for updating values there, instead of these variables. I do not recall any other variables that change headers or message body. The note should be clear enough not to introduce more confusion.

@xkaraman
Copy link
Contributor Author

xkaraman commented Aug 20, 2024

Regarding format fix: Here are all the rules that are being checked.

The current disabled checks are MD013: false MD046: false, meaning line length and code block style. All the rest are on the default config according to markdownlint-cli2.yaml. It makes sense to have the file's contents be consistent, and some tools are enforcing and can automagically fix most of these rules. When someone modifies a file that has not been fixed, can just run the tool to fix it (or have a github action do it if possible). Otherwise, we should disable the checks we do not want in the markdownlint-cli2.yaml.

Regarding the notes: Updated the respected pseudo-variables and recommended uac functions instead.

docs/tutorials/faq/main.md Outdated Show resolved Hide resolved
docs/tutorials/faq/main.md Outdated Show resolved Hide resolved
@miconda
Copy link
Member

miconda commented Aug 21, 2024

I added two comments, afterwards it can be merged.

Reading more about the markdown lint tool that is used in the pre-commit action, it enforces only a subset of markdown, because it considers to be more consistent. Even markdown allows -, + or * for unordered list items, the lint tool wants only one to be used. I saw there were couple of options to configure for it, but my basic tests failed to get ok for the sublist mode, which should be ok with the ToC as it was before this PR (with a different character on each sublist level). I find this style better because one can identify the level easier, but you can merge and have - everywhere in the ToC. For more about the styles, see:

Maybe my dev environment was not set properly, specific nodejs seems to be required by the markdown linting action.

And for everyone that does not know, if you want to commit and the pre-commit checks forbids it, but you are ok with what is done (e.g., because the check is too strict and can be omitted), one can skip the check with --no-verify option:

git commit --no-verify ...

@xkaraman
Copy link
Contributor Author

I modified the markdownlint.yaml to accept (enforce really) the sublist different characters.

Comments are also resolved.

@miconda
Copy link
Member

miconda commented Aug 23, 2024

I am ok to merge it, thanks!

The lint tool seems to be opinionated, only one option possible, although the markdown allows combination, that's fine after all.

@xkaraman xkaraman merged commit 5679a52 into kamailio:main Aug 23, 2024
1 check passed
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.

2 participants