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

Add grid_attrs argument to Model constructor #297

Merged
merged 6 commits into from
Nov 13, 2018

Conversation

spencerkclark
Copy link
Collaborator

Closes #182

@spencerahill
Copy link
Owner

Thanks for speeding this through @spencerkclark! I should have time on Friday to properly review this.

Copy link
Owner

@spencerahill spencerahill left a comment

Choose a reason for hiding this comment

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

With the caveat that I'm a little rusty at thinking carefully about the codebase, I actually think the implementation looks good as is. A couple minor docs suggestions is all.

aospy/test/data/objects/examples.py Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
@spencerahill
Copy link
Owner

spencerahill commented Nov 13, 2018

Awesome --- this looks ready to go @spencerkclark . Merge whenever you're ready, and thanks as always for the great work.

@spencerkclark
Copy link
Collaborator Author

@spencerahill one more minor update for you to have a look at; I added a reference table to the documentation showing the current built-in alternative names (I think it's a bit tricky for new users to find those at the moment). Let me know if that looks good and I'll go ahead and merge.

Copy link
Owner

@spencerahill spencerahill left a comment

Choose a reason for hiding this comment

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

Great idea on the table. One tiny edit; go ahead and merge once that's done.

docs/examples.rst Outdated Show resolved Hide resolved
@spencerkclark
Copy link
Collaborator Author

Thanks for the review @spencerahill; merging now!

@spencerkclark spencerkclark merged commit e6287fa into spencerahill:develop Nov 13, 2018
@spencerkclark spencerkclark deleted the fix-182 branch November 13, 2018 18:54
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.

None yet

2 participants