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

Template other files #17

Merged
merged 6 commits into from
May 15, 2019
Merged

Template other files #17

merged 6 commits into from
May 15, 2019

Conversation

hunse
Copy link
Collaborator

@hunse hunse commented Apr 23, 2019

Motivation and context:

We want templating for other files.

How has this been tested?

Currently just hand-testing on some other repos. PRs for other repos will reference this issue and appear below as they're created.

How long should this take to review?

  • Average (neither quick nor lengthy)

Types of changes:

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

Still to do:

  • contributing
  • manifest
  • setup.cfg
  • docs/conf.py
  • setup.py
  • Add documentation for all this
  • Add tests for all this
  • Do we want nengo-bones header on rst files?
  • Update this repository's .nengobones.yml and regenerate files

@hunse hunse force-pushed the template-other-files branch 4 times, most recently from fe9b416 to c5af726 Compare April 24, 2019 18:12
@arvoelke
Copy link
Contributor

Did we also want to template out pull_request_template.md?

@hunse
Copy link
Collaborator Author

hunse commented May 2, 2019

So one reason I made some config options like "author" appear both in the highest-level config and sub-configs like license_config, rather than passing both config and license_config to the license template like 2a46d2b does, is so that license can overwrite the author if it wants. That way we could have e.g. "Applied Brain Research" as the author in setup.py, and "Applied Brain Research, Inc." as the author in LICENSE.rst.

@drasmuss
Copy link
Member

drasmuss commented May 2, 2019

Yeah I was thinking about that, but my vote would be to keep the configuration options as simple as we can. So unless we find a really compelling reason where we'd need to override top-level config options, just set those things in one place. I don't think we've needed that flexibility yet, so this way we just have a single source of truth for, e.g., the author value.

@hunse
Copy link
Collaborator Author

hunse commented May 2, 2019

I did use it for "description" in Nengo, because we have a longer description in setup.py than in the docs. But we can probably figure out a shorter description for Nengo that will work everywhere.

@drasmuss
Copy link
Member

drasmuss commented May 2, 2019

Yeah I'm just looking through all the other repos now to see if any of my fixups broke anything (I'm sure they did), will take a look.

@hunse
Copy link
Collaborator Author

hunse commented May 2, 2019

One question I had when writing this is what do we want to do with the __copyright__ variable currently in __init__.py in some of our repos. It'd be nice if this didn't have to be updated manually, but it seems like a pain to add templating to __init__.py just for this. Can we just get rid of these? The main place I saw them used was in docs/conf.py, and we got rid of the use there.

Copy link
Member

@drasmuss drasmuss 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 (I made some fairly extensive fixups, so we'll get another review on this too). I added a card for the __init__ copyright issue, figure we won't worry about that here.

nengo_bones/config.py Outdated Show resolved Hide resolved
Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

I made some changes in squash and fixup commits, most of which are minor, but 97559e9 has some more substance to it as it changes how we are overriding templates in downstream repos. I've also squashed history down a fair bit and I think it looks good as is, but if you want some more squashing of the later commits @drasmuss I can squash more liberally (e.g., merging the "Test Python 3.7" and "Add flake8" commits into "Apply templating for new files").

I'm going to fix up the downstream PRs while you look these changes over @drasmuss, and will merge when those are working as expected and you've given the 👍.

Copy link
Member

@drasmuss drasmuss left a comment

Choose a reason for hiding this comment

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

Fixups look good! Just two minor comments.

nengo_bones/scripts/generate_bones.py Outdated Show resolved Hide resolved
.nengobones.yml Outdated Show resolved Hide resolved
drasmuss and others added 6 commits May 15, 2019 18:51
The following files can now be generated through `generate-bones`:

- docs/conf.py
- CONTRIBUTING.rst
- CONTRIBUTORS.rst
- LICENSE.rst
- MANIFEST.in
- setup.cfg
- setup.py

These files are also checked through the `check-bones` command.

Additionally, files with no config line are no longer generated.
All files now need at least an empty config line
or they will be skipped.

Co-authored-by: Eric Hunsberger <eric.hunsberger@appliedbrainresearch.com>
Co-authored-by: Trevor Bekolay <tbekolay@gmail.com>
@tbekolay tbekolay merged commit 07514ed into master May 15, 2019
@tbekolay tbekolay deleted the template-other-files branch May 15, 2019 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants