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

Update library description #55

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

hasenradball
Copy link
Contributor

Update function descriptions

  • add function documentation

update Doxygenfile

  • add Doxygen file for docu build

update Latex Documentation

  • add files after latex build

Hi Rob,
I am not sure if is worth to keep all the files from latex build.

@RobTillaart RobTillaart self-assigned this Dec 3, 2024
@RobTillaart RobTillaart added the documentation Improvements or additions to documentation label Dec 3, 2024
@RobTillaart
Copy link
Owner

RobTillaart commented Dec 3, 2024

Please remove the output of doxygen (HTML + LATEX), It takes unneeded time to review / click.
Just want to know that it can be generated, and if not it should give a failed build.

Did some homework last night and created this workflow (not 100%), it should build the documentation.
Still needs success / fail flag and clean up the generated output.

to be added under .github/workflows

doxygen.yml

name: DOXYGEN

on: [push, pull_request]
jobs:
  doxygen:
    runs-on: ubuntu-latest
    timeout-minutes: 10
    steps:
      - uses: actions/checkout@v4
      - uses: mattnotmitt/[email protected]
        with:
          working-directory: '.'
          doxyfile-path: 'doxyfile'
      - run: |
          ls -alR

@hasenradball
Copy link
Contributor Author

hasenradball commented Dec 3, 2024

So also not to keep the generated PDF?

revove also th folders
doxygen
html
latex
?

@RobTillaart
Copy link
Owner

I missed the PDF in the 100+ files,
As it is only one file, yes, keeping one PDF doesn't clutter my disks

Propose to put it the doxygen folder instead of in latex

@RobTillaart
Copy link
Owner

RobTillaart commented Dec 3, 2024

Now I have 100+ new files to review, that just doesn't work for me.


update
eh oops, deleted files no problemo

@RobTillaart
Copy link
Owner

Can you add that doxygen workflow I proposed above to see if and how it works?

@hasenradball
Copy link
Contributor Author

I will think will not work.

what is doxyfile path?

@RobTillaart
Copy link
Owner

what is doxyfile path?

The path to the doxyfile starting from the base of the checked out repo.

See - https://github.com/mattnotmitt/doxygen-action

@RobTillaart
Copy link
Owner

@hasenradball
Thanks for all your work,

The doxygen action ran successfully (don't know if it can fail yet)

  • the files are generated based upon the doxyfile, so people can create their HTML TEX whatever.
  • ls -AlR part gives way to much output ==> should become ls -alR doxigen

In short, I am quite happy so far, thanks again

To be continued later (want to finish some other stuff today too)

@RobTillaart
Copy link
Owner

@hasenradball

Q: why has the .cop a copy of all doxy comments?
This is doing work twice, which is very inefficient and error prone from maintenance point if view.

@RobTillaart
Copy link
Owner

@hasenradball

Merge conflict comes from merging your other PRs.

@hasenradball
Copy link
Contributor Author

Good morning Rob,

thank you for merging my PRs and for the confidence.
I will check the conflicts and greate a latest pdf, then.
What do you mesn with the cop file was not aware.

best Frank

PS: when merging the confllicts should I increase the version number?

@hasenradball
Copy link
Contributor Author

If you don‘t mind I see similar improvements for the MCP23008.
😊

@RobTillaart
Copy link
Owner

What do you mesn with the cop file was not aware.

I meant the PCF8574.cpp file.

It is sufficient to create the doxy comments only in the PCF8574.h file.
All functions and variables are defined in the header file.

1 similar comment
@RobTillaart
Copy link
Owner

What do you mesn with the cop file was not aware.

I meant the PCF8574.cpp file.

It is sufficient to create the doxy comments only in the PCF8574.h file.
All functions and variables are defined in the header file.

@RobTillaart
Copy link
Owner

 when merging the confllicts should I increase the version number?

Can do, should be done eventually anyway.

As there is no functional change in the code the version should become 0.4.2

@hasenradball
Copy link
Contributor Author

hasenradball commented Dec 5, 2024

comments in the .h

Hi Rob,
yeah the best way is to have the whole comments in header file, wen can do that.
No problem, but be aware that you don' t see the whole function deklaration in one shot.

But normally the best is to have the doxy comments in header.
And then keep the cpp file clean.
Should we do so?
And waht about the File description?
Also have it in one file?
Or keep it in both.

I would also think about the version number. It is 3 times defined (2 times in Header one time in cpp)

Please give me a sign what to do:
a) with doxy comments
b) with gitignore

One more hint #pragma once is non standard but widely used better is to use include guards like

#ifndef
#def


#endif

Should we keep #pragma ?

Best frank

@RobTillaart
Copy link
Owner

RobTillaart commented Dec 5, 2024

In short

  • only doxy comments in the PCF8574.h file. That is sufficient. Keep the PCF8574.cpp clean.

  • no gitignore file in the repo as it is environment and user specific.

  • the three versions are by design. One printable string PCF_LIB_VERSION and two in the code file header so people can check easily if they belong to each other. (costs me days to fix once to find out that people mixed up version of .cpp and .h)

  • pragma once can be left as is. There are never bug reports reported about it, so no reason to change it. Yes people tell once or twice a year that it is non standard officially, but in practice it is a de facto standard.


No problem, but be aware that you don' t see the whole function deklaration in one shot.

Don't know what you mean by that. Can you provide screenshots or ? to explain.


And waht about the File description?

Does it add value somehow when added in the .cpp file? The information stays the same.

@RobTillaart
Copy link
Owner

I call it a day, have been working for almost 15 hours today,
Tomorrow will be a busy day again.

@hasenradball
Copy link
Contributor Author

Hi Rob,

did an update now.

please have a look updated the ignore file, would suggest to keep it like this, otherwise you will have 100 or more files in your git area to stage. It is better keep it like this.

  • updated gitignore file
  • removed all documentation for function in cpp file => copied to header file
  • files looks clean and better now :-)
  • updated pdf

@hasenradball
Copy link
Contributor Author

I am not 100% sure about the file description, but it is sufficient to keep Filedescription in *.h and remove it from *.cpp file.

@hasenradball
Copy link
Contributor Author

So take a look I think it is very nice now.
My own libs don't look so perfekt :-)

@RobTillaart
Copy link
Owner

Good morning,

Insight of last night is to put the .gitignore file itself in .gitignore.

That allows you to have it as you need, no clutter with 100+ files staged, and every other user can have its own version (or none) without disruptive interference.

Opinion?

@hasenradball
Copy link
Contributor Author

Good morning,

did not finally catch what you mean by this:
gitignore file itself in .gitignore.

@RobTillaart
Copy link
Owner

If you put the filename .gitignore into .gitignore, it will be used by the git commands, but not appear in e.g. git status

@hasenradball
Copy link
Contributor Author

ah understood what you mean.

putting itself gitignore file into .gitignore.
Never did before but ok for me.

@hasenradball
Copy link
Contributor Author

Added .gitignore to the file itself.
But I think it will not work.
What was your intention with this?

@RobTillaart
Copy link
Owner

My intention is to have no .gitignore in the repo.
.gitignore is user or environment specific.
It does not belong in a repo as other users might get conflicts.

By adding . gitignore into .gitignore you can ignore all files you want but the git commands will not include .gitignore in any commit.

@RobTillaart
Copy link
Owner

You still have to do git rm .gitignore as it was committed earlier

@hasenradball
Copy link
Contributor Author

hasenradball commented Dec 6, 2024

Sorry I did not understood correctly.
You don't want to have gitignore in the repo.
But you point was to keep locally, right.
So I did not under stand correctly, what you mean

@hasenradball
Copy link
Contributor Author

Sorry for the misunderstanding.

// URL: https://github.com/RobTillaart/PCF8574
// http://forum.arduino.cc/index.php?topic=184800


#include "PCF8574.h"
Copy link
Owner

Choose a reason for hiding this comment

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

Lost the header of the cpp file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@hasenradball
Copy link
Contributor Author

No you mentioned in the Discussion it brings no benefit to keep.
So please make clear statements.

@RobTillaart
Copy link
Owner

I had a quick review, hope to find more time coming weekend, but at the moment very busy.

most comments are rather small, I think you did a very good job sofar!

@RobTillaart
Copy link
Owner

No you mentioned in the Discussion it brings no benefit to keep.
So please make clear statements.

Sorry my mistake to not being clear enough.

@hasenradball
Copy link
Contributor Author

Hi,

any news for this PR?

@hasenradball
Copy link
Contributor Author

Hello Rob,

could you have a look to this PR?

@RobTillaart
Copy link
Owner

@hasenradball

I'm ill and am not able to work.
All my activities stopped.
Be aware this might take some weeks.
Sorry for the inconveniences,

@hasenradball
Copy link
Contributor Author

hasenradball commented Dec 14, 2024

Hi Rob,

oh my god sorry for that.
Wish you a soon recovery.
Will wait for your feedback then.

One point I had in mind with doxygen. You will be able with github pages to update the github page when push into the main branch.
Do you have expirience in that point?

Szenario would be that if main branch has a push a doxy run will update the github Docu page.

@hasenradball
Copy link
Contributor Author

Here you see an example of automated Documentation creation.

AM2302-Sensor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants