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

Document content definition tags #967

Merged
merged 5 commits into from Sep 14, 2016

Conversation

Projects
None yet
3 participants
@dbenage-cx
Member

dbenage-cx commented Sep 12, 2016

From forum thread

Specific tags are currently undocumented, leaving those trying to reference them to hunt for the current intended use.
Some method to address documenting these would likely help even the more experienced members.

Anyone focusing on a specific section (e.g. AI) will likely want a reference to tags in each of the others.

Maintaining a central file (or 4 files for compiled / definition content / python content / AI) seems a burden, so I tried implementing with doxygen.

Since doxygen is not really designed to document the values of sparsely placed variables, the output is not exactly as I'd hope, though it is functional.

A key change here is setting CREATE_SUBDIRS = NO which results in a different file output and layout. Restoring to YES results in a broken link at the definition point ("Definition Tag" -> tag group page), at least in my locally generated version (doxygen v1.8.11).
If changing is not acceptable, the list will still generate and link to the references correctly, the user will just need to use the menu for a list.

Also added default/python to the documented files, with a filter to only document python files with a tag entry.

docu_tags_all
Not certain why CTRL_EXTINCT links twice, when CTRL_ALWAYS_REPORT does not.

docu_tags_local

Documenting tags within the FOCS definitions will probably require writing some rule to parse them (if moving to python goes well, then not an issue).

Are there any alternate ideas roaming around (or pointers for those with a better handle on doxygen)?

@adrianbroher adrianbroher self-assigned this Sep 12, 2016

@adrianbroher adrianbroher changed the title from [WIP][Feedback] Document content definition tags to Document content definition tags Sep 12, 2016

@adrianbroher adrianbroher added this to the post 0.4.6 milestone Sep 12, 2016

Show outdated Hide outdated doc/Doxyfile.in Outdated
@adrianbroher

This comment has been minimized.

Show comment
Hide comment
@adrianbroher

adrianbroher Sep 12, 2016

Contributor

Looks good in general.

Is there a particular reason why you add sections to the tags? I don't see much value into separating 'core' and 'mod' tags.

Since doxygen is not really designed to document the values of sparsely placed variables, the output is not exactly as I'd hope, though it is functional.

You're talking about the duplicated entries, right?

If changing is not acceptable, the list will still generate and link to the references correctly, the user will just need to use the menu for a list.

It's not like I will fight for keeping CREATE_SUBDIRS set to YES if it's causing bugs, I just liked the doc root directory to be clean as possible. But it seems like the xrefitem keyword doesn't work well with CREATE_SUBDIRS, at least there are some bugs in the doxygen bugtracker that go in that direction.

Contributor

adrianbroher commented Sep 12, 2016

Looks good in general.

Is there a particular reason why you add sections to the tags? I don't see much value into separating 'core' and 'mod' tags.

Since doxygen is not really designed to document the values of sparsely placed variables, the output is not exactly as I'd hope, though it is functional.

You're talking about the duplicated entries, right?

If changing is not acceptable, the list will still generate and link to the references correctly, the user will just need to use the menu for a list.

It's not like I will fight for keeping CREATE_SUBDIRS set to YES if it's causing bugs, I just liked the doc root directory to be clean as possible. But it seems like the xrefitem keyword doesn't work well with CREATE_SUBDIRS, at least there are some bugs in the doxygen bugtracker that go in that direction.

@dbenage-cx

This comment has been minimized.

Show comment
Hide comment
@dbenage-cx

dbenage-cx Sep 12, 2016

Member

Is there a particular reason why you add sections to the tags?

I was imagining content creators documenting a custom resource directory. That seems like a long term prospect, so agree it should lose the separation.

You're talking about the duplicated entries, right?

The duplicated entry is a minor annoyance imo, there may not be a viable solution to it.

I initially envisioned, and was trying for:

Section` title

SOME_TAG_1 brief description
SOME_TAG_2 brief description

And then at the definition point:

SOME_TAG_1 brief description
detailed description

Member

dbenage-cx commented Sep 12, 2016

Is there a particular reason why you add sections to the tags?

I was imagining content creators documenting a custom resource directory. That seems like a long term prospect, so agree it should lose the separation.

You're talking about the duplicated entries, right?

The duplicated entry is a minor annoyance imo, there may not be a viable solution to it.

I initially envisioned, and was trying for:

Section` title

SOME_TAG_1 brief description
SOME_TAG_2 brief description

And then at the definition point:

SOME_TAG_1 brief description
detailed description

@dbenage-cx

This comment has been minimized.

Show comment
Hide comment
@dbenage-cx

dbenage-cx Sep 13, 2016

Member

The duplication occurs for any additional use within the same scope.
I've worked around this by moving those entries into const variables.

Comments in python are apparently not parsed for this alias if the comment is indented. Adjusted filter to strip the spacing for these, so as not to break pep8 adherence.

Would a rebranch with reduced commits be preferred?

Member

dbenage-cx commented Sep 13, 2016

The duplication occurs for any additional use within the same scope.
I've worked around this by moving those entries into const variables.

Comments in python are apparently not parsed for this alias if the comment is indented. Adjusted filter to strip the spacing for these, so as not to break pep8 adherence.

Would a rebranch with reduced commits be preferred?

Show outdated Hide outdated doc/tag_filter.py Outdated
Show outdated Hide outdated doc/All_Tags.dox Outdated
Show outdated Hide outdated UI/EncyclopediaDetailPanel.cpp Outdated
@adrianbroher

This comment has been minimized.

Show comment
Hide comment
@adrianbroher

adrianbroher Sep 13, 2016

Contributor

Would a rebranch with reduced commits be preferred?

You can rebase within this branch.

https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history

Contributor

adrianbroher commented Sep 13, 2016

Would a rebranch with reduced commits be preferred?

You can rebase within this branch.

https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history

@dbenage-cx

This comment has been minimized.

Show comment
Hide comment
@dbenage-cx

dbenage-cx Sep 13, 2016

Member

Short on time, will sort out the commits later tonight/in morning.

Member

dbenage-cx commented Sep 13, 2016

Short on time, will sort out the commits later tonight/in morning.

dbenage-cx added some commits Sep 12, 2016

@dbenage-cx

This comment has been minimized.

Show comment
Hide comment
@dbenage-cx

dbenage-cx Sep 13, 2016

Member

Rebased. Trying for cleaner final commits, open to any criticism.

Member

dbenage-cx commented Sep 13, 2016

Rebased. Trying for cleaner final commits, open to any criticism.

@adrianbroher

This comment has been minimized.

Show comment
Hide comment
@adrianbroher

adrianbroher Sep 14, 2016

Contributor

The PR is working as expected and looks good. Well done.

Contributor

adrianbroher commented Sep 14, 2016

The PR is working as expected and looks good. Well done.

@adrianbroher adrianbroher merged commit 74c7723 into freeorion:master Sep 14, 2016

@adrianbroher

This comment has been minimized.

Show comment
Hide comment
@adrianbroher

adrianbroher Sep 14, 2016

Contributor

Trying for cleaner final commits, open to any criticism.

I'm happy with the outcome. The only criticism I have is that the implementation of doc/tag_filter.py could be simpler, but that's no reason to hold back the PR. You're reading the input file twice when you could output the matching lines in the first read. An implementation like:

# Output files only if they contain an element we want to document

import sys
import os

file_name = sys.argv[1]
file_obj = open(file_name, 'r')

tag_comment = "content_tag{"

# filter for matching tags
for line in file_obj:
    if tag_comment in line:
        # doxygen wont document this alias if it is indented
        print line.strip()
file_obj.close()

would do the same as yours with half of the code to maintain.

Contributor

adrianbroher commented Sep 14, 2016

Trying for cleaner final commits, open to any criticism.

I'm happy with the outcome. The only criticism I have is that the implementation of doc/tag_filter.py could be simpler, but that's no reason to hold back the PR. You're reading the input file twice when you could output the matching lines in the first read. An implementation like:

# Output files only if they contain an element we want to document

import sys
import os

file_name = sys.argv[1]
file_obj = open(file_name, 'r')

tag_comment = "content_tag{"

# filter for matching tags
for line in file_obj:
    if tag_comment in line:
        # doxygen wont document this alias if it is indented
        print line.strip()
file_obj.close()

would do the same as yours with half of the code to maintain.

@dbenage-cx

This comment has been minimized.

Show comment
Hide comment
@dbenage-cx

dbenage-cx Sep 14, 2016

Member

Thank you for reviewing and assist.

reading the input file twice when you could output the matching lines in the first read

I originally went this route (and commenting out lines instead of no output), but without context in the file, doxygen ignored the tag.

  • If it is ensured to document the file itself, duplication may be a issue later.
  • Unless there is an existing reliable method of catching the containing scope, trying to parse for one seemed highly error-prone.
  • Adding/removing certain lines is warned against for INPUT_FILTER, so inserting some fake context (despite the commentators intent) is problematic.
Member

dbenage-cx commented Sep 14, 2016

Thank you for reviewing and assist.

reading the input file twice when you could output the matching lines in the first read

I originally went this route (and commenting out lines instead of no output), but without context in the file, doxygen ignored the tag.

  • If it is ensured to document the file itself, duplication may be a issue later.
  • Unless there is an existing reliable method of catching the containing scope, trying to parse for one seemed highly error-prone.
  • Adding/removing certain lines is warned against for INPUT_FILTER, so inserting some fake context (despite the commentators intent) is problematic.

@dbenage-cx dbenage-cx deleted the dbenage-cx:document_content_tags branch Sep 14, 2016

@adrianbroher

This comment has been minimized.

Show comment
Hide comment
@adrianbroher

adrianbroher Sep 14, 2016

Contributor

Adding/removing certain lines is warned against for INPUT_FILTER, so inserting some fake context (despite the commentators intent) is problematic.

But that's still no reason to read the file twice.

# Output files only if they contain an element we want to document

import sys
import os

file_name = sys.argv[1]
file_obj = open(file_name, 'r')

tag_comment = "content_tag{"

# dedent doxygen tag documentation
for line in file_obj:
    if tag_comment in line:
        # doxygen wont document this alias if it is indented
        print line.strip()
    else:
        print line
file_obj.close()
Contributor

adrianbroher commented Sep 14, 2016

Adding/removing certain lines is warned against for INPUT_FILTER, so inserting some fake context (despite the commentators intent) is problematic.

But that's still no reason to read the file twice.

# Output files only if they contain an element we want to document

import sys
import os

file_name = sys.argv[1]
file_obj = open(file_name, 'r')

tag_comment = "content_tag{"

# dedent doxygen tag documentation
for line in file_obj:
    if tag_comment in line:
        # doxygen wont document this alias if it is indented
        print line.strip()
    else:
        print line
file_obj.close()
@dbenage-cx

This comment has been minimized.

Show comment
Hide comment
@dbenage-cx

dbenage-cx Sep 14, 2016

Member

Sorry if I'm being dense, won't this output all given files?
I'm not arguing for or against doing so here, I assumed adding them all was undesired.

Could fill up a buffer and only print if a match was found.
Still iterating the data twice when there is a match, but avoids a second file operation.

Member

dbenage-cx commented Sep 14, 2016

Sorry if I'm being dense, won't this output all given files?
I'm not arguing for or against doing so here, I assumed adding them all was undesired.

Could fill up a buffer and only print if a match was found.
Still iterating the data twice when there is a match, but avoids a second file operation.

@Cjkjvfnby

This comment has been minimized.

Show comment
Hide comment
@Cjkjvfnby

Cjkjvfnby Jan 29, 2017

Contributor
  • Use with statement
  • Use iterator over file line for line in file
  • Read file in one cycle
Contributor

Cjkjvfnby commented on 9b8388a Jan 29, 2017

  • Use with statement
  • Use iterator over file line for line in file
  • Read file in one cycle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment