-
Notifications
You must be signed in to change notification settings - Fork 12
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
Function to retrieve inherited object data implementation in nce.py #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to my comments, please resolve the issues pointed out by the checks. Most of them can be fixed automatically using tox
(see the contribution guidelines for more information).
The 2nd, 3rd and 4th items of the PR checklist apply for this PR (but I can imagine you are still working on those, since this PR was submitted only recently).
Hi @crnh, Regarding the tox stuff, could you give me more details on how to use it? At the moment, I only know how to make pull requests by modifying the files through GitHub UI on my fork in my browser. I'm pretty sure its very archaic, but that's all I know. I did install tox, but I don't even know how to use my own fork to connect the ZOSAPI. Take care, David |
Hi @Omnistic, Sure! In order to run the unit tests etc., you need to clone the repository to your local file system. You can find instructions for that when you click the green "code" button in your fork. Many editors (e.g. VS Code) also offer an option to clone a git repository, which is even easier. Once the repository has been cloned to your PC, you can run the Feel free to ask us any questions if anything is unclear! |
Hi @crnh, The short story is that tox failed, but is quite cryptic for me to read. The long story is that I cloned my fork of the repository, from which I made the pull request, using Sourcetree. I then opened a CMD.exe prompt from Anaconda Navigator (Windows) from an environment, in which I installed tox (that environment also has ZOSPy, I hope its not a problem). From that command prompt, I navigated to my local cloned repository and executed tox. There's a lot of text, perhaps the relevant part is this one:
There seem to be a lot of UnicodeDecodeError when I browse through the text. Can you advise on what to do next? Sorry I'm a bit clueless at this point. Take care, David |
Hi, Thanks for the update! Sounds like it was a good idea to run the unit tests, as some of them are failing. I do not expect this to be related to your contribution. Can you please share the full tox output (either by pasting it here or by using a pastebin)? Then I can check what's causing this issue. |
Regarding your setup: installing tox in an environment with ZOSPy installed should be fine, as tox creates its own environment in which it runs the tests. |
Show test output
|
Hi @crnh, I've pasted the tox result above. Also, I reently updated to R2.00. I hope this helps, and take care, David |
Thanks for sharing the output! The problem seems related to parsing the text output from certain analyses. I'll split this to a separate issue, to keep this PR clean. |
Co-authored-by: crnh <30109443+crnh@users.noreply.github.com>
Thanks @crnh, I'm starting to have a grasp of what these tests mean. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed change
Type of change
Additional information
Related issues
Checklist
If you contributed an analysis:
AttrDict
s for the analysis result data (please use dataclasses instead).If you contributed an example: