bengeisler / TcLog

Flexible logging functionality for TwinCAT 3.
MIT License
54 stars 10 forks source link

feat: some smaller changes; removed lineids; a little bit of code styling #6

Closed seehma closed 1 year ago

seehma commented 1 year ago

Hi Ben, first, thanks for this package! I have made some smaller improvements:

bengeisler commented 1 year ago

Hi Matthias, thanks for contributing and your valuable feedback!

I'd like to discuss enums instead of constants for library projects, though. Why would you consider enums a better fit? I deliberately chose local constants since the states are only used for the internal state machines within one method of the respective FB and do not get reused anywhere else.

Pro enum:

Contra enum:

There are also local enums, however, they seem to come with their own drawbacks so I decided not to use them for now.

Code styling and naming conventions are an opinionated but important topic, thanks for bringing it up! I'm happy to use your proposal. To keep it consistent for future changes of this project, I think we should set up a code formatter and static analysis for this project. Are you using a specific formatting tool? And do you have a preferred static analysis configuration that enforces your style guide? The rules that you can set to the Beckhoff PLC Static Analysis seem to be limited to the Beckhoff style guide.

seehma commented 1 year ago

Hi, thanks for your reply. sorry i forgot to make those enums internal

i share all your pro arguments and want to add some

Its a good idea, we have an internal tool which is currently not shareable. We also tried some static analysis tools and are working on a new one, but iam not sure until when we can release that for public use. Regards M.

bengeisler commented 1 year ago

The code has been updated to the proposed Zeugwerk naming conventions with 3591818. This introduced breaking changes in several method signatures.

It now also uses TcBlack for formatting.

bengeisler commented 1 year ago

Hi Matthias,

I have some questions about the Zeugwerk naming conventions where the documentation remains unspecific:

seehma commented 1 year ago

Hi @bengeisler , at the very top of the coding conventions we have a short list (cheatsheet), where we tried to sumarize the most important "rules".

For method inputs and outputs (if you use outputs, not returns) (e.g. VAR_INPUT, VAR_OUTPUT) these should be lower camelCase -> as it is in C#, you are right, we have a lot of our Conventions from C#.

For FBs or PRGs as these are Public members in VAR_INPUT and VAR_OUTPUT we use PascalCase. The difference to VAR_INPUTs of methods is, that for methods these are not members, for FBs these stay on the heap.

For PRG members we also use PascalCase, because in ST these are publicly accessible, Those in FBs are not, therefore we use a _prefix here. This makes it also easier to quickly see the difference with auto completion.

In our Framework-Libraries we use the name of the object as main folder and then it depends a little. Most of the time there are some Datatypes (DUTs as it is called in ST) and for these we add a folder _DataTypes (even if it is only one). For Tests, we usually write our tests in the same solution in order to keep it simple and easy to use, we have a folder _Test. With interfaces it is a bit more complex. our framework is designed to be platform independet (in future), therefore interfaces are located in the ZCore library (with no dependency at all) and are used in every library which is above ZPlatform (from a hierarchy perspective) e.g. ZAux, ZEquipment,... For the interface-naming we use ILogger -> I as a Prefix.

If you like i can do it for your project in a new PR and then you can decide if you like it!? Also let me know if you have any troubles with zkdoc. Regards M.

bengeisler commented 1 year ago

Thanks for the clarification! I'm currently updating the comments to the zkdoc style and I'd just update the method arguments to camelCase right with it. I changed them all to PascalCase in the last commit 😄

Once I'm done with that I'd put the tests in the same project and update the folder names. The leading _ of the folder names stands for internal objects?

I guess there is no preview feature for zkdoc, am I right? I can only see it after the commit.

seehma commented 1 year ago

ok, sorry for that, i will see if we can write it more specific in our coding conventions. Also a good feedback for us :-). A lot of datatypes are only used internally in our libraries but there are also some which are not. One reason is, that the folder is always located at the very top of the objects folder (listing after names in ascending order).

Go into my forked TcLog repository -> https://github.com/seehma/TcLog and on the right you can click on gh-pages, there you can see how it will look after our ci/cd system pushes the documentation.

grafik

If you want, we also offer a build service for twincat projects. It will build your library and execute unit tests (if there are some). The unit tests can (as said before) be right beside your code (in the same project) and we saw that then developers love it more to write unit tests and keep them up to date (i'm talking only of PLC-unit tests).