chore: update pre-commit#184
Conversation
|
@danielsirakov same here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 71.63% 71.74% +0.11%
==========================================
Files 25 25
Lines 3437 3437
==========================================
+ Hits 2462 2466 +4
+ Misses 975 971 -4
🚀 New features to boost your workflow:
|
|
For some weird reason this is deleting blank lines after the docstring. I asked claude and it seems as if this is a bug in v1.7.8 of docformatter which people have requested fixes for. Please could you try again (let's close this PR so we don't merge all the back and forth history) pinning to 1.7.7 and see what happens? |
|
@danielsirakov could you revisit this one as we have an agreement that deleting line form |
|
The files look Ok. We just need to check if this version is passing pre-commit |
|
@danielsirakov Could you take a look for the failure of |
Sounds good, I'll take a look after I make the last few edits to diffpy.structure |
…rfit into update_pre-commit # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
|
I was able to bypass the docformatter and black conflict by changing triple quotes to just using double quotes in 3 files. Let me know if this is an appropriate workaround. @stevenhua0320, ready to review |
|
@danielsirakov I think we have one left for the conflict here: |
I think this had something to do with me merging my edits with the bot edits; I just ran the pre-commit, and it edited the file automatically and passed once I ran it again. I'll push again with the pre-commit's edit. |
|
@stevenhua ready to review |
|
@danielsirakov Still not passing, first you could do the |
Sorry about that, it said it was passing when I ran the pre-commit before pushing so I’m not sure why that’s happening. I’m not home right now, but I’ll take a look as soon as I’m back. |
sbillinge
left a comment
There was a problem hiding this comment.
I left comments. Let's try moving the failing module level docstrings to the top
| Exceptions used for SrFit - specific errors. | ||
| """ | ||
|
|
||
| "Exceptions used for SrFit - specific errors." |
There was a problem hiding this comment.
This one should be a docstring I think. Does it work putting it right at the top of the file but with triple quotes?
There was a problem hiding this comment.
Sorry for the long delay; I've been having technical difficulties with my computer. When I try this, the docformatter and black conflict happens once again. Are there any other workarounds you recommend I should try?
There was a problem hiding this comment.
Could you describe how does it conflict with each other, in other words, what does black do initially?
There was a problem hiding this comment.
It seems like black is adding a blank line before class, while docformatter is removing it
There was a problem hiding this comment.
I believe this is the same scenario that we had before, try to add a #FIXME comment and see if we could get around this.
There was a problem hiding this comment.
just pushed, let me know if everything looks fine
@stevenhua0320 ready to review
There was a problem hiding this comment.
One more fix is needed, look into the tests/test_characteristicfunctions.py and see how does black and docformatter is conflicting right now. Also don't stay up too late haha.
There was a problem hiding this comment.
I'll take a look right now. Don't worry, I'm gonna hit the hay after this : )
There was a problem hiding this comment.
It looked to be the same scenario as before, and I added a #FIXME line. Let me know if it looks fine
There was a problem hiding this comment.
Interesting, it looks like black and the pre-commit-ci bot have conflict, as the bot removes a line that black adds. What do you recommend we do? Claude recommended I move the import to the top of the file instead of inside the function.
| # | ||
| ############################################################################## | ||
| """Universal import functions for volatile SasView/SansViews API-s.""" | ||
| "Universal import functions for volatile SasView/SansViews API-s." |
There was a problem hiding this comment.
This should be a docstring, try moving to the top
| # | ||
| ############################################################################## | ||
| """Functions for binding arguments of callable objects.""" | ||
| "Functions for binding arguments of callable objects." |
…rfit into update_pre-commit
| + "latest sasview API" | ||
| ) | ||
| from sasmodels.sasview_model import find_model, load_standard_models | ||
|
|
There was a problem hiding this comment.
Could we move this import statement to the top of import modules and check whether it could fix the conflicting behavior while the test module is still valid?
There was a problem hiding this comment.
I moved it to the top but I noticed there are other instances of the import statements being in that location, should I delete them all since I have it in the import section now?
There was a problem hiding this comment.
Also, how do I check that the test module is still valid?
There was a problem hiding this comment.
If they import the same function, you could remove it and test it by running pytest on your terminal; otherwise, you need to add the function name eg:find_model after the import and rerun pytest to see whether the test is running properly.
There was a problem hiding this comment.
I'm having trouble running pytest as I don't have the necessary packages downloaded in my environment, and when I tried downloading them, it said that the diffpy-srfit package required Python between 3.11 and 3.13. Should I make a new environment, or try and test it in a different way?
There was a problem hiding this comment.
btw, I'll finish this tomorrow as I'm going to bed
There was a problem hiding this comment.
btw, I'll finish this tomorrow as I'm going to bed
No worries, have a good rest for today!
There was a problem hiding this comment.
Thanks! You too!
There was a problem hiding this comment.
You need to have this installed in editable mode to do testing, but this is standard practice, we should maybe have told you. I also am seeing the CI pre-commit working target hard. Out Is Better of you are running pre-commit locally. We should only see CI pre-commit things very rarely when things go wrong locally. @Rundong can help you get everything set up locally correctly
There was a problem hiding this comment.
Sounds good, I'll reach out to Rundong!
|
@danielsirakov please make sure that tests are passing locally before pushing. Did you run pytest on everything on your local? It won't catch everything, but anything that fails locally will also fail in CI. This would, most likely, have failed locally. |
|
@danielsirakov It might be because you installed And hence we could get around this. Additionally, when we are doing this, please open an issue for this repo. The title should briefly describe the problem that we have right now, and then on the main block we could try to come up a potential solution (eg: get some of our dependency out of srfit). Thanks for doing that. |
Sorry for the delay; I was traveling and moving furniture today. I ran pytest locally before pushing, and it passed, so I'm not sure why it's still failing. I'll go back and double-check now, and then I'll add Rundong's try except fix and test again. Once I'm done, I'll open an issue and try to work on a solution with Rundong. |
Yes. This happens sometimes. The CI runs tests on a clean environment and locally your tests are running in whatever environment you have on your machine. The CI tests are therefore stricter and a better test of the user experience. |
What is meant by a clean environment, and could I make a new environment that matches the one CI tests on in my local to check before pushing? |
|
Yes. Just create a new 3:14 environment and install whatever is in |
|
Then run pytest in that env |
|
thanks, I'll try that now |
|
When I ran pytest, it wouldn't run because pkg_resources was missing. Is that something I should install, or does this show that it won't work in CI's clean environment? |
|
You may want to work with Rundong as I didn't think pkg_resources was a dependency in or latest code so there may be a problem with your installation. |
It might be good to reset the environment that you are running for this package. To do this, first we should have a clean environment that has nothing except for python. Then activate it Assuming you have added and also install the test: Up till now, we have rebuilt the environment. To test whether our edit is proper, after your edits, you need to run and then run |
When I run pip install -e . in my Python 3.14 environment, it fails with ERROR: Package 'diffpy-srfit' requires a different Python: 3.14.6 not in '<3.14,>=3.11'. I thought editable mode would bypass this, but it doesn't seem to. Should I update the requires-python constraint in pyproject.toml to include 3.14, or is there another workaround? |
Sorry, I forgot this package still not pushing to |
|
With the try except workaround, it passes pytest in the clean environment but only with the extend path in init.py, without the edits it cannot find diffpy.srfit. Should I try pushing with or without the extend path edit to init.py? |
Also, it passes the pre-commit in the clean environment. |
You should then commit the changes without |
Sounds good, I'll push now |
…rfit into update_pre-commit
|
@stevenhua0320 ready to review |
|
@danielsirakov Did you confirm that all pytest is passing now? If so please paste the result here and if so I would call Simon to take the review. |
|
I just double checked again now. Everything passes, but test_characteristicfunctions.py was skipped. I originally thought this was because my environment didn't have sasmodels installed, but I downloaded and tried again, and they were still skipped. I looked into it, and there's a skip message in each of the tests that says "sas characteristic functions not currently working, remove skip when our code is refactored." |
|
I originally assumed the pytest output meant everything was passing, but now I realize the tests themselves are marked to skip intentionally, so pytest isn't actually running the test logic to verify that the functions work and only verifies that the import statement doesn't crash. |
Yes, and our convention is that when you go through the test, you need to show the reviewer the output. It is okay that you don't do it this time, but from now on, it is good to paste the test output with a code block. For example: @sbillinge ready for another review. |
|
Sorry for not pasting it, I just wasn't sure if we needed to run it again due to the skips. Here's my most recent pytest: `(ci_pytest_env) PS C:\Users\danny\dbs\diffpy.srfit> pytest tests\test_builder.py ...... [ 4%] ========================================================================= warnings summary ========================================================================= tests/test_diffpyparset.py::testDiffpyStructureParSet tests/test_diffpyparset.py::test___repr__ tests/test_diffpyparset.py::test___repr__ -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html |
Try to do the triple quote next time, which would display the result correctly. |
|
I see. Thanks for the help! |
|
Thanks for this @danielsirakov great job. That was more work than we expected but hopefully some good learning was accomplished in the process! Please work with @stevenhua0320 to do the steps to get the next version released. |
|
Thank you, I definitely learned a lot! Looking forward to working with Rundong on the next release. |
@stevenhua0320, ready to review