Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Enable the comment in model config files. #262

Merged
merged 1 commit into from
Oct 25, 2022

Conversation

AsakusaRinne
Copy link
Contributor

@AsakusaRinne AsakusaRinne commented Jun 18, 2022

Fix: #258, #263

What does it solve

It add support for writing comments in model config files.

What does it change

It add a new const variable to indicate the comment symbol '#' and remove the part after comment symbol in the string in AddDef.

@casbin-bot
Copy link
Member

@sagilio @xcaptain @huazhikui please review

@casbin-bot
Copy link
Member

@sagilio @xcaptain @huazhikui please review

@AsakusaRinne
Copy link
Contributor Author

@sagilio The comment itself is not affected by the model config. However, model config contents are necessary to test it. So I just add comments to an existing model config test file. Do you think that writing a dependent file to test the comments is better, or just keeping it is enough?

@hsluoyz
Copy link
Member

hsluoyz commented Jun 19, 2022

@sagilio

@sagilio
Copy link
Member

sagilio commented Jun 19, 2022

@sagilio The comment itself is not affected by the model config. However, model config contents are necessary to test it. So I just add comments to an existing model config test file. Do you think that writing a dependent file to test the comments is better, or just keeping it is enough?

I think we need to write new example files and tests for the comments test, and the test also needs to cover the entire line of comments.

This will also more helpful to others format support in future.

@sagilio sagilio self-assigned this Jun 19, 2022
@sagilio sagilio added the bug Something isn't working label Jun 19, 2022
@hsluoyz
Copy link
Member

hsluoyz commented Jun 19, 2022

@AsakusaRinne

  1. Don't mess with the existing models. Try to fork a new model from RBAC and test all your supported comments on that file only.
  2. Make sure to align with the Golang Casbin, you should NOT create a comment style that are not compatible with Golang Casbin's model. If you are doing so, try to get help from the community to let Golang Casbin support it first (or make PR to Golang Casbin by yourself if you know Go). Finally all Casbin implementations will support the SAME commenting style and Golang Casbin should be the first to support it, then port to others.

@AsakusaRinne AsakusaRinne changed the title fix: enable the comment in model config files. [WIP] fix: enable the comment in model config files. Jun 19, 2022
@AsakusaRinne
Copy link
Contributor Author

@AsakusaRinne

  1. Don't mess with the existing models. Try to fork a new model from RBAC and test all your supported comments on that file only.
  2. Make sure to align with the Golang Casbin, you should NOT create a comment style that are not compatible with Golang Casbin's model. If you are doing so, try to get help from the community to let Golang Casbin support it first (or make PR to Golang Casbin by yourself if you know Go). Finally all Casbin implementations will support the SAME commenting style and Golang Casbin should be the first to support it, then port to others.

OK, I got it, I have signed this PR as WIP and will complete the tips above before requesting a merge.

@AsakusaRinne AsakusaRinne changed the title [WIP] fix: enable the comment in model config files. [WIP] fix: Enable the comment in model config files. Jun 19, 2022
@sagilio
Copy link
Member

sagilio commented Jun 19, 2022

@AsakusaRinne

  1. Don't mess with the existing models. Try to fork a new model from RBAC and test all your supported comments on that file only.
  2. Make sure to align with the Golang Casbin, you should NOT create a comment style that are not compatible with Golang Casbin's model. If you are doing so, try to get help from the community to let Golang Casbin support it first (or make PR to Golang Casbin by yourself if you know Go). Finally all Casbin implementations will support the SAME commenting style and Golang Casbin should be the first to support it, then port to others.

@hsluoyz golang version also supports the inline comment:
https://github.com/casbin/casbin/blob/5225b867429740e5ffdb69a124c3890f1a66a4f9/examples/comment_model.conf#L1-L12

@AsakusaRinne AsakusaRinne changed the title [WIP] fix: Enable the comment in model config files. fix: Enable the comment in model config files. Jun 20, 2022
@hsluoyz
Copy link
Member

hsluoyz commented Aug 29, 2022

@AsakusaRinne fix:

image

@AsakusaRinne
Copy link
Contributor Author

@sagilio Do you think we still need this fix?

Copy link
Member

@sagilio sagilio left a comment

Choose a reason for hiding this comment

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

LGTM

@sagilio sagilio merged commit a6f1604 into casbin:preview Oct 25, 2022
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

🎉 This PR is included in version 2.0.0-preview.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
4 participants