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.

Sign up for GitHub

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

Jump to bottom

Symbol tables were added to def's that did not have them #819

Merged
merged 15 commits into from
Nov 24, 2020
Merged

Symbol tables were added to def's that did not have them #819

merged 15 commits into from
Nov 24, 2020

Conversation

RussBerg
Copy link
Contributor

@RussBerg RussBerg commented Oct 19, 2020 •
edited by prjemian
Loading

also a standard is proposed for a symbol naming convention as many definitions refer to a similar symbol "meaning" but the naming was arbitrary, number of points for example (nP, numP, NumP etc).

Fixes #800, fixes #786, fixes #787, fixes #788

@RussBerg
Symbol tables were added to def's that did not have them
83c6322 
also a standard is proposed for a symbol naming convention as many definitions refer to a similar symbol "meaning" but the naming was arbitrary, number of points for example (nP, numP, NumP etc).
@RussBerg RussBerg added this to the NXDL 2020.10 milestone Oct 19, 2020
@RussBerg
Copy link
Contributor Author

This is a work in progress, do not merge yet

@RussBerg RussBerg marked this pull request as ready for review October 19, 2020 16:02
@RussBerg RussBerg requested review from a team October 19, 2020 16:34
@RussBerg
Copy link
Contributor Author

@PeterC-DLS I couldn't see which comments you made, to which lines do your comments apply?

@RussBerg
Copy link
Contributor Author

does this PR require a NIAC vote in order to be merged?

This was referenced Oct 19, 2020
Closed
Closed
@RussBerg RussBerg linked an issue Oct 19, 2020 that may be closed by this pull request
NXstxm: missing rank value in dimensions specification #787
Closed
@RussBerg
added new symbol dataRank
180b39a 
adjusted the rank to be the expression 1+dataRank and  added to the docstring for NXdetector
PeterC-DLS
PeterC-DLS requested changes Oct 20, 2020
View reviewed changes
applications/NXindirecttof.nxdl.xml Outdated Show resolved Hide resolved
applications/NXfluo.nxdl.xml Outdated Show resolved Hide resolved
applications/NXiqproc.nxdl.xml Outdated Show resolved Hide resolved
applications/NXiqproc.nxdl.xml Outdated Show resolved Hide resolved
applications/NXlauetof.nxdl.xml Outdated Show resolved Hide resolved
applications/NXtofnpd.nxdl.xml Outdated Show resolved Hide resolved
applications/NXtomo.nxdl.xml Outdated Show resolved Hide resolved
applications/NXtomophase.nxdl.xml Outdated Show resolved Hide resolved
applications/NXtomoproc.nxdl.xml Outdated Show resolved Hide resolved
applications/NXxbase.nxdl.xml Outdated Show resolved Hide resolved
@RussBerg RussBerg requested review from mkoennecke and a team and removed request for a team October 20, 2020 15:14
prjemian
prjemian requested changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@prjemian prjemian left a comment

Choose a reason for hiding this comment

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

comments as noted

@prjemian prjemian mentioned this pull request Oct 20, 2020
Closed
@benajamin benajamin mentioned this pull request Oct 23, 2020
Closed
@prjemian
Copy link
Contributor

travis-ci has identified an indentation problem in NXxas.nxdl.xml:

python ../../../../utils/nxdl2rst.py NXxas.nxdl.xml > NXxas.rst
Traceback (most recent call last):
  ...
  File "../../../../utils/nxdl2rst.py", line 93, in getDocBlocks
    re.sub(r'\t',"\\\\t", line ),
Exception: Bad indentation in <doc> of None [		    The symbol(s) listed here will be used below to coordinate datasets
			with the same shape.
        ]: expected "\t\t    " found "\t\t\twith the same shape.".
Makefile:49: recipe for target 'NXxas.rst' failed

@RussBerg RussBerg marked this pull request as draft November 3, 2020 15:45
@RussBerg RussBerg marked this pull request as ready for review November 5, 2020 21:13
prjemian
prjemian approved these changes Nov 5, 2020
View reviewed changes
Copy link
Contributor

@prjemian prjemian left a comment

Choose a reason for hiding this comment

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

LGTM, despite the couple off indentation items as reported. Since they only affect the XML formatting and not documentation, leave them be.

PeterC-DLS
PeterC-DLS requested changes Nov 6, 2020
View reviewed changes
applications/NXmonopd.nxdl.xml Outdated Show resolved Hide resolved
applications/NXrefscan.nxdl.xml Outdated Show resolved Hide resolved
applications/NXtofnpd.nxdl.xml Outdated Show resolved Hide resolved
applications/NXtofraw.nxdl.xml Outdated Show resolved Hide resolved
applications/NXlauetof.nxdl.xml Outdated Show resolved Hide resolved
@prjemian prjemian mentioned this pull request Nov 6, 2020
Open
@RussBerg RussBerg requested a review from PeterC-DLS November 19, 2020 15:22
@RussBerg RussBerg removed the request for review from mkoennecke November 19, 2020 15:58
PeterC-DLS
PeterC-DLS requested changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@PeterC-DLS PeterC-DLS left a comment

Choose a reason for hiding this comment

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

Just a couple of blips left.

applications/NXrefscan.nxdl.xml Outdated Show resolved Hide resolved
applications/NXreftof.nxdl.xml Outdated Show resolved Hide resolved
@RussBerg RussBerg merged commit 479e9fb into master Nov 24, 2020
@RussBerg RussBerg deleted the 800-symbols_are_not_defined branch November 24, 2020 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benajamin benajamin benajamin approved these changes

@PeterC-DLS PeterC-DLS PeterC-DLS requested changes

@prjemian prjemian prjemian approved these changes

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
NXDL 2020.10
4 participants
@RussBerg @prjemian @PeterC-DLS @benajamin