Skip to content

Stop attempting to convert previously-converted doc headers #1005

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

Merged
merged 2 commits into from
Jun 18, 2017

Conversation

reuk
Copy link
Contributor

@reuk reuk commented Jun 12, 2017

Updates the documentation conversion script so that it doesn't try to convert file headers which have already been converted. This should mean that, after the initial run, subsequent runs of the do_doc_convert script do not make any changes to the codebase, which should make rebases from pre-Doxygen code much easier.

reuk added a commit to reuk/cbmc that referenced this pull request Jun 13, 2017
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Looks good though I think one line is little confusing - will try it out anyway and report back.

if header_formatter.needs_new_header(file_contents) and converted:
return '%s%s' % (
block_contents.group(0),
header_formatter.convert(header_from_block(block)) + '\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I am misunderstanding something, can we not use converted here. Why do we now need the check converted is truthy? Surely if it is an empty string this amounts to returning block_contents.group(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, I'll fix that now.

@reuk
Copy link
Contributor Author

reuk commented Jun 13, 2017

Should be fixed now.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

LGTM - but I will test it

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

LGTM - but I will test it works for my use case.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

It works - the approach that works for me is to do a rebase on the current base (i.e not master) first. I will document this on the wiki and share on slack.

@thk123
Copy link
Contributor

thk123 commented Jun 13, 2017

When this is merged this documentation can be simplified to just say "grab latest version of the script": https://github.com/diffblue/doc-diffblue-common/wiki/Rebasing-PRs-after-the-Doxygen-Change

@martin-cs
Copy link
Collaborator

When I try to run the script as a stand-alone program (to do help update a branch) it seems to hang:

$ reformat_docs.py --help 
^C/tmp/reformat_docs.py: line 3: syntax error near unexpected token `('
/tmp/reformat_docs.py: line 3: `Field = collections.namedtuple('Field', ['name', 'contents'])'

$ reformat_docs.py analyses/constant_propagator.h
^C/tmp/reformat_docs.py: line 3: syntax error near unexpected token `('
/tmp/reformat_docs.py: line 3: `Field = collections.namedtuple('Field', ['name', 'contents'])'

@reuk
Copy link
Contributor Author

reuk commented Jun 15, 2017

Interesting - this suggests that it doesn't like namedtuple for some reason, but that's been in Python since v2.4. Can you open an interpreter and run

import collections
Field = collections.namedtuple('Field', ['name', 'contents'])

@reuk
Copy link
Contributor Author

reuk commented Jun 15, 2017

Ignore what I said above. The script is not designed to be run directly (which is why it was not marked executable). If you want to run it in that way, you should add #!/usr/bin/env python as the first line of the file, or alternatively run it using python reformat_docs.py.

@reuk
Copy link
Contributor Author

reuk commented Jun 15, 2017

I updated the wiki page with a bit more info about actually running the conversion scripts.

@martin-cs
Copy link
Collaborator

martin-cs commented Jun 15, 2017 via email

@kroening kroening merged commit 17e5963 into diffblue:master Jun 18, 2017
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