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

Ess adding in house functionalities #10

Merged
merged 13 commits into from
Mar 14, 2022

Conversation

nitrosx
Copy link
Member

@nitrosx nitrosx commented Feb 17, 2022

THis pull adds to the official main branch in the SciCat project the changes that we have implemented in-house at ESS.

The code is still a work in progress and needs mroe testing.

@nitrosx
Copy link
Member Author

nitrosx commented Feb 17, 2022

I forgot about the linting. Will work on it and update this PR

Copy link
Member

@dylanmcreynolds dylanmcreynolds left a comment

Choose a reason for hiding this comment

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

Awesome stuff, thanks!

I added a couple of small comments.

Also, we should add unit tests for your new methods.

https://github.com/SciCatProject/pyscicat/blob/main/pyscicat/tests/test_client.py

There's not a ton of logic in there, but the requests mock code can at least verify that we're pointing to the correct URL and test what logic is in there.

pyscicat/model.py Show resolved Hide resolved
pyscicat/client.py Show resolved Hide resolved
@dylanmcreynolds
Copy link
Member

Also, can you please look into adding documentation at https://github.com/SciCatProject/pyscicat/tree/main/docs/source? Maybe a couple of how to documents about getting proposals and instruments? I think the API calls you added will get auto generated, but we should check.

@nitrosx
Copy link
Member Author

nitrosx commented Feb 21, 2022

I will look into the documentation

@nitrosx
Copy link
Member Author

nitrosx commented Feb 24, 2022

@dylanmcreynolds : I'm going to test pyScicat in a use case. I will have the examples to insert in the documentation once I'm done with that. I think it will take few days.
Do you want to wait for that to merge or should we go ahead merge and do another PR when I have the examples ready?

@dylanmcreynolds
Copy link
Member

@dylanmcreynolds : I'm going to test pyScicat in a use case. I will have the examples to insert in the documentation once I'm done with that. I think it will take few days. Do you want to wait for that to merge or should we go ahead merge and do another PR when I have the examples ready?

I'm happy to wait. Thanks!

@nitrosx
Copy link
Member Author

nitrosx commented Mar 8, 2022

@dylanmcreynolds : I updated the documentation as you requested.

@nitrosx nitrosx requested a review from dylanmcreynolds March 8, 2022 12:46
@nitrosx
Copy link
Member Author

nitrosx commented Mar 9, 2022

@dylanmcreynolds : is it ok if we merge this PR and I work and include testing in a new PR?

@dylanmcreynolds
Copy link
Member

is it ok if we merge this PR and I work and include testing in a new PR?

I would much prefer to add unit tests now.

@dylanmcreynolds
Copy link
Member

Thanks for the documentation!

The way it is right now, your hard work is not yet viewable in the documentation.

Check out this

```{toctree}

We could either add your documents as an and entry in "How To", or make a new category in the TOC called "Examples" for this documentation. What do you think?

BTW, this should be better documented, but to test the documentation build locally:

If you have not done so yet in your environment, setup the dev requirements to get the sphinx build code:

pip install -r requirements-dev.txt

Then:

cd docs
make html

This will build the web site locally into docs/build/html, and you can open a browser to the index.html

…n of documentation, updated example. Added packages in requirements to requirements-dev.
@nitrosx
Copy link
Member Author

nitrosx commented Mar 10, 2022

@dylanmcreynolds : I added our example under the "How-to" section.
I fixed the json code in the document, so it builds.
I also added packages pydantic and requests in requirements-dev.txt, so to build the documentation the following instructions will suffice:

pip install -r requirements-dev.txt\n
cd docs  
make html  

I think that with this we addressed all the issues.
Please let me know

@dylanmcreynolds
Copy link
Member

Sorry, my instructions assumed that you had already run:

pip install -r requirements.txt

People tend treat requirements-dev.txt as "add-ons to the primary package". I'd rather not have dependencies repeeated in different files. Can you please remove this?

@nitrosx
Copy link
Member Author

nitrosx commented Mar 11, 2022

No problem. Done!!!
We should include this info in the documentation. New ticket?

@dylanmcreynolds
Copy link
Member

We should include this info in the documentation. New ticket?

Yes, good idea:
als-computing/pyscicat#6

@nitrosx nitrosx requested a review from dylanmcreynolds March 14, 2022 10:49
@dylanmcreynolds dylanmcreynolds merged commit 399e349 into main Mar 14, 2022
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.

2 participants