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

XDMF Scripts: Fix Replacements & Offset #2309

Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Oct 16, 2017

Fix the particle offset naming since the introduction of openPMD and fix missing .format() replacements with multi-line strings.

The latter issue was introduces with #2145, the former with release 0.2.0.

Fix the particle offset naming since the introduction of openPMD
and fix missing ".format()" replacements with multi-line strings.
@ax3l ax3l added affects latest release a bug that affects the latest stable release bug a bug in the project's code component: tools scripts, python libs and CMake labels Oct 16, 2017
@ax3l ax3l added this to the 0.4.0 / 1.0.0: Next Stable milestone Oct 16, 2017
Copy link
Member

@PrometheusPi PrometheusPi 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 to me.
I would recommend a plus sign to combine strings, but that is optional.
Your name is missing in the license header, please indicate wether you want to add it or leave it as it is. I will merge after your decision.

node_text = "/Xdmf/Domain/Grid/{}Attribute[@Name='{}']/" + \
"DataItem[1]".format(extra_grid, orig_name)
node_text = ("/Xdmf/Domain/Grid/{}Attribute[@Name='{}']/"
"DataItem[1]").format(extra_grid, orig_name)
Copy link
Member

Choose a reason for hiding this comment

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

OPTIONAL:
A plus sign + is optional here but would improve readability of the code to emphasize that here the second string is attached.

Copy link
Member

Choose a reason for hiding this comment

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

same below

Copy link
Member Author

Choose a reason for hiding this comment

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

I would not add it since it is superfluous like additional brackets.
the way it is now is the common way to do it properly.

Copy link
Member

Choose a reason for hiding this comment

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

Okay

@PrometheusPi PrometheusPi self-assigned this Oct 17, 2017
@ax3l ax3l changed the title XDMF Scripts: Fix Replacemends & Offset XDMF Scripts: Fix Replacements & Offset Oct 18, 2017
@PrometheusPi PrometheusPi merged commit c34fc30 into ComputationalRadiationPhysics:dev Oct 18, 2017
@ax3l ax3l deleted the fix-xdmfNewlineFormat branch October 19, 2017 07:10
@ax3l ax3l mentioned this pull request Oct 19, 2017
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects latest release a bug that affects the latest stable release bug a bug in the project's code component: tools scripts, python libs and CMake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants