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 doxygen config file: add protocol and update tag #1851

Merged
merged 1 commit into from
Aug 12, 2019

Conversation

oxarbitrage
Copy link
Member

@oxarbitrage oxarbitrage commented Jul 17, 2019

As now we have protocol outside chain library we need to add it to DoxyFile in order to generate docs for it.

Also, as current develop already haves more code than 3.2.0 i updated the tag to 3.3.0 as IMO current and future develop will actually became 3.3.0.

#741 can be closed by this.

@abitmore
Copy link
Member

We don't have a libraries/chain/db directory.

@abitmore abitmore added this to the 3.3.0 - Feature Release milestone Jul 17, 2019
@abitmore abitmore changed the title add protocol and update tag Update doxygen config file: add protocol and update tag Jul 17, 2019
@abitmore
Copy link
Member

abitmore commented Jul 17, 2019

I guess we want to add libraries/db, libraries/net, libraries/plugins and etc as well.

And FC.

@oxarbitrage
Copy link
Member Author

I think is a good idea even if we dont have doxygen friendly documentation everywhere, we might have in the future.

For a demo on how it will look with everything please check: https://open-explorer.io/doxygen/html/

If we do this, we will be in position to close #741

@abitmore
Copy link
Member

abitmore commented Jul 17, 2019

Looking at https://open-explorer.io/doxygen/html/annotated.html, it seems it's a bit too much. Perhaps exclude docs of external projects e.g. websocketpp?

@oxarbitrage
Copy link
Member Author

i excluded external projects, here is how it is looking now: https://open-explorer.io/doxygen/html/annotated.html

We might want to merge squashed if we get into something here.

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

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

Looks better now. On https://open-explorer.io/doxygen/html/annotated.html, why the boost category is empty?

@@ -793,7 +793,7 @@ RECURSIVE = YES
# Note that relative paths are relative to the directory from which doxygen is
# run.

EXCLUDE =
EXCLUDE = libraries/fc/vendor/editline libraries/fc/vendor/secp256k1-zkp libraries/fc/vendor/websocketpp
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's better to put this in fc's Doxyfile

Copy link
Member Author

Choose a reason for hiding this comment

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

It will not work like that if we want have the fc included in bitshares-core output.
doxygen will not read the DoxyFile at sub folders.

We have 3 options:

  • remove the DoxyFile from FC fully.
  • add the excludes to Doxyfile in FC in case someone is building the documentation only for FC. We still will want to keep the excludes here as we want FC to be part of bitshares-core output.
  • dont modify Doxyfile in FC and produce the external modules output if documentation is being generated for FC only.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. I think the 2nd and 3rd are both fine.

@abitmore
Copy link
Member

On https://open-explorer.io/doxygen/html/annotated.html, why the boost category is empty?

@oxarbitrage
Copy link
Member Author

I removed the boost from the namespaces at 822bc45, i think it can be a good alternative. Including all boost libraries into our project is massive.

https://open-explorer.io/doxygen/html/annotated.html

@abitmore
Copy link
Member

Looks like it would be better if we move some classes into namespaces. @pmconrad @nathanhourt @jmjatlanta what do you think?

@oxarbitrage
Copy link
Member Author

There are several warnings in the doxygen generation about undefined references in the codebase, etc. Probably all easy to solve.
We may want to go over these in a separated issue. I will post a doxygen output as soon as we define what are the pieces we will have included(aka after this pull request is merged).

@nathanielhourt
Copy link
Contributor

Looks like it would be better if we move some classes into namespaces. @pmconrad @nathanhourt @jmjatlanta what do you think?

Sorry, I don't follow you... What classes need to be moved into namespaces? AFAIK pretty much all of our classes are in a graphene::xyz namespace, except maybe some unit testing code...

@abitmore
Copy link
Member

Looks like it would be better if we move some classes into namespaces. @pmconrad @nathanhourt @jmjatlanta what do you think?

Sorry, I don't follow you... What classes need to be moved into namespaces? AFAIK pretty much all of our classes are in a graphene::xyz namespace, except maybe some unit testing code...

@nathanhourt click https://open-explorer.io/doxygen/html/annotated.html you'll see quite some classes are out of graphene::.

@pmconrad
Copy link
Contributor

Some of these are only inside .c files (e. g. mz_*, deduplicator), some in tests (my_io_class). Should these be documented at all? They are not part of the library API.

@oxarbitrage
Copy link
Member Author

By adding this file patterns 712bbc5 we remove all the stuff coming from c files.

Please check: https://open-explorer.io/doxygen/1851/html/annotated.html

@abitmore
Copy link
Member

I didn't save a screenshot of the old page, so don't know what have changed. Here is a screenshot of current page:
image

I'd say we merge this PR for this release then continue improving it with a new PR in next release. As follow-up, I created #1895 for moving classes into namespaces.

abitmore
abitmore previously approved these changes Aug 10, 2019
@oxarbitrage
Copy link
Member Author

It seems the squash and merge option is not available in this repo. Should we merge with all the commits ? I think it will be a good idea to have that enabled for some cases.

@abitmore
Copy link
Member

You can manually squash them.

git rebase -i origin/develop

It would open a text editor, you leave the first commit with pick, change all other picks to squashs, then force-push.

@abitmore
Copy link
Member

After squashed, perhaps better use git commit --amend to update the commit message.

@oxarbitrage
Copy link
Member Author

Thank you @abitmore , just did that.

@oxarbitrage
Copy link
Member Author

need your approval again :)

@oxarbitrage oxarbitrage merged commit cd09370 into develop Aug 12, 2019
@abitmore abitmore deleted the oxarbitrage-patch-1 branch August 14, 2019 19:56
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.

4 participants