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

WIP: Added docgen command #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

gmelillo
Copy link
Contributor

@gmelillo gmelillo commented Dec 6, 2018

This new command can be used to generate markdown documentation for any rubiks setup.
If run with modules all the objects included into the modules will be automatically added inside the generated markdown.

the markdown will be placed inside docs/rubiks.class.md

TODO:

  • Define if the file position is ok.
  • Define if all the section naming is appropriate.
  • Define if we like to ship in this merge request all the class docstrings or if it will be better to send them on another PR.
  • Define the transformation type. (at the moment all transformation are unknown transformation)

Sections

  • Formats
    • Base64
    • Command
    • Confidential
    • JSON
    • YAML
  • Types
    • ARN
    • Boolean
    • CaseIdentifier
    • ColonIdentifier
    • Domain
    • Enum
    • IP
    • IPv4
    • Identifier
    • Integer
    • List
    • NonEmpty
    • NonZero
    • Nullable
    • Number
    • Path
    • Positive
    • String
    • SurgeSpec
    • SystemIdentifier
    • WildcardDomain
  • Objects
    • Too many to be there :)

Copy link
Contributor

@mb-m mb-m left a comment

Choose a reason for hiding this comment

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

This is mostly pretty good, though there are a couple of blockers here, particularly: why the change to load_python.py, and let's have a better description fallback. If you really have to put a "TODO" which I don't agree with for the reasons I've stated (I also think that VarTypes of Base64 and JSON are pretty self-explanatory), then you should at least put the class name in there.

self.get_repository(can_fail=True)
objs = load_python.PythonBaseFile.get_kube_objs()
types = load_python.PythonBaseFile.get_kube_types()
formats = load_python.PythonBaseFile.get_kube_vartypes()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I particularly like the name "formats" for vartypes.

header += '# Table of contents\n\n'

header += '- [Formats](#formats)\n'
md += '\n# Formats\n\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

This structure makes sense if you add options (which I think you should) for which sections you should generate.

And as mentioned, I think "formats" is an awful name for this.

for fname in sorted(formats.keys()):
header += ' - [{}](#{})\n'.format(fname, fname.lower())
md += '## {}\n\n'.format(fname)
md += '{}\n\n'.format(formats[fname].get_description())
Copy link
Contributor

Choose a reason for hiding this comment

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

particularly because of this!


with open(os.path.join(r.basepath, 'docs/rubiks.class.md'), 'w') as f:
f.write(header.encode('utf-8'))
f.write(md.encode('utf-8'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, please fix your editor so that the file ends in a newline and we don't get spurious diffs down the line.

def get_description(cls):
if cls.__doc__ is not None and cls.__doc__ is not '':
return '\n'.join([line.strip() for line in cls.__doc__.split('\n')])
return 'TODO: Description is still missing from the class docstring.\nStay tuned to have more hint about this variable.'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this text - after all, Boolean/Integer etc should speak for themselves. I think this should be:

return cls.__name__

@@ -515,6 +537,7 @@ def cluster_info(c):

ret.update(self.__class__.get_kube_objs())
ret.update(self.__class__.get_kube_vartypes())
ret.update(self.__class__.get_kube_types())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed!? Why do you think you need, say "Integer" or "String" in the DSL?

if cls.__doc__ is not None and cls.__doc__ is not '':
return '\n'.join([line.strip() for line in cls.__doc__.split('\n')])
return 'TODO: Description is still missing from the class docstring.\nStay tuned to have more hint about this variable.'

Copy link
Contributor

Choose a reason for hiding this comment

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

Again

return cls.__name__

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.

2 participants