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

Minor typo and formatting fixes for symmetries #486

Merged
merged 4 commits into from
Jan 4, 2024

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Dec 20, 2023

Just a few things that popped up whilst implementing the new symmetry operations.

I think it's better to rearrange the big block descriptions into the "Requirements/conventions" sections to follow how we do it with other fields (see the second commit here) but happy to roll that one back.

@ml-evs ml-evs changed the title Minor typo and formatting fixes Minor typo and formatting fixes for symmetries Dec 23, 2023
merkys
merkys previously approved these changes Jan 2, 2024
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

I support moving conventions under "Requirements/Conventions". Other changes are small, nevertheless appropriate.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
merkys
merkys previously approved these changes Jan 4, 2024
Copy link
Contributor

@vaitkus vaitkus left a comment

Choose a reason for hiding this comment

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

I had an additional look at the rendered version of the file and the indentation of some lists seems a bit off (see marked suggestions).

optimade.rst Show resolved Hide resolved
optimade.rst Show resolved Hide resolved
optimade.rst Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
ml-evs and others added 2 commits January 4, 2024 11:16
Co-authored-by: Antanas Vaitkus <[email protected]>

- Apply suggestions from code review

Co-authored-by: Antanas Vaitkus <[email protected]>

- Update optimade.rst

Co-authored-by: Antanas Vaitkus <[email protected]>
merkys
merkys previously approved these changes Jan 4, 2024
vaitkus
vaitkus previously approved these changes Jan 4, 2024
Copy link
Contributor

@vaitkus vaitkus 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, feel free to ignore the two newly added nitpicky comments.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@ml-evs ml-evs dismissed stale reviews from vaitkus and merkys via fb20e6e January 4, 2024 12:01
@ml-evs ml-evs requested review from merkys and vaitkus January 4, 2024 12:02
@ml-evs
Copy link
Member Author

ml-evs commented Jan 4, 2024

Thanks @merkys and @vaitkus, hopefully this is the last request ;)

Copy link
Contributor

@vaitkus vaitkus left a comment

Choose a reason for hiding this comment

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

Thanks!

@ml-evs ml-evs merged commit 2518fee into develop Jan 4, 2024
5 checks passed
@ml-evs ml-evs deleted the ml-evs/formatting-fixes branch January 4, 2024 12:09
@vaitkus vaitkus mentioned this pull request Jan 4, 2024
Comment on lines -3750 to +3745
Symmetry operation strings that comprise the :property:`space_group_symmetry_operation_xyz` property MUST conform to the following regular expressions.
Symmetry operation strings that comprise the :property:`space\_group\_symmetry\_operations\_xyz` property MUST conform to the following regular expressions.
Copy link
Contributor

@rartino rartino Jan 5, 2024

Choose a reason for hiding this comment

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

Is quoting underscores inside custom roles in rst necessary? This footnote leads me to think that it is not?: https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#footnote-reference-3

Some testing does seem to suggest it doesn't really harm anything for custom roles, so I guess it is ok to do it. In literal context (double backticks) the backslashes are rendered.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main fix here was the typo in the field name, perhaps I was over cautious with the \...

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