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

Flake fixes pt4 #127

Closed
wants to merge 12 commits into from
Closed

Flake fixes pt4 #127

wants to merge 12 commits into from

Conversation

cadenmyers13
Copy link
Contributor

No description provided.

@cadenmyers13
Copy link
Contributor Author

@sbillinge Okay, I think that takes care of all the flake8 errors. My brain went numb towards the end so let me know if there's anything that I can fix

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

now we are getting into things that can break the code, let's set up CI so we can see what tests are breaking on each PR.

So, before we do this flake8 work, let's move over the .github workflows and get CI passing on the PRs. On a separate PR, merge the .github directory from the cookiecutter (sorry I don't remember if you ran the cookiecutter yet). If you didn't run the cookiecutter yet becaus ewe are linting first, just coy the github workflows from diffpy.pdfmorph and edit them appropriately.

Nice work on the exceptions. It is a balance between being more precise (i.e. ValueError or whatever) rather than (Exception) but not changing the behavior.... If it was blank before, then for sure Exception isn't making it worse, but if we can figure out what error might need to be handled it is better. I see that you did good digging on that.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

please revert the copyright statements in this PR (please see the comment in the other PR I closed.

@cadenmyers13
Copy link
Contributor Author

cadenmyers13 commented Jul 26, 2024

please revert the copyright statements in this PR (please see the comment in the other PR I closed.

Are these not reverted back on your end? I don't see the copyright edits appearing anymore on this PR.

@sbillinge
Copy link
Contributor

yes, they are reverted. Sorry, too much going on.

Please can you merge cookie into your branch for this PR so we can see if tests are stlil passing.

Also, please can you show me the flake8 errors that caused you to make the RealMenu changes? This may change the behavior of the code, so we need to be super-sure on this. It seems to be beyond linting somehow.

cadenmyers13 and others added 5 commits July 26, 2024 17:32
* pin numpy to 1.x for now

* add environment.yml

* test tests

* trigger

* try again with requirements

* try again with requirements

* now update test reqs

* try and handle xwindow display issue

* try using xfvb action

* refactor xvfb action commands

* source activate?

* different tack, generate error and work on test?

* again

* again

* chatgpt method

* supress running qc of meta.yaml in conda-forge by renaming

* try new pyproject.toml

* again2

* forgot to install diffpy.pdfgui!

* back to setup.py
@sbillinge
Copy link
Contributor

unfortunately this is not passing the tests (please look in Details then User xfvb action and you will see some E's there instead of dots. Not sure why, so we may have to look at those before merging.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

We may have to go more slowly with this.

let's try

  1. make a new PR from a syned cookie with just the copyright statements. This doesn't change any of the behavior so we can check that all the tests pass in ci. Then we can try and add in the code changes one at a time and make sure the tests pass locally. To run the tests locally type python -m run_tests.py in the top level directory.
  2. For some reason it is exiting with an error, but the tests run ok because we see only dots in the tests and no E's or F's, so the exit erro (we would like to track this down and fix it but don't spend time on it now because we will change how we run the tests after cookiecuttering so want to make everything work then....for now we just need to know that we haven't done anything to break the code while flake8'ing) we can ignore for now.
  3. Do the code edits you did before (exceptions etc.) one at a time and rerun tests. This way we can see which edits are breaking the code.
  4. If the tests are passing, commit.
  5. Make periodic pushes of working code, which will run CI on GitHub, and make sure that you are getting the same behavior there as you got locally, i.e., no E's or F's
  6. I can merge anything that has only dots. If you are strugging with one or two last things, lets put them on a separate PR so we can merge the working-code changes.

@@ -54,7 +54,7 @@ test:
# entry points work.


# You can also put a file called run_test.py in the recipe that will be run
# You can also put a file called dont_run.py in the recipe that will be run
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not needed but these hacks will be removed later so don't worry...

@cadenmyers13
Copy link
Contributor Author

cadenmyers13 commented Jul 29, 2024

yes, they are reverted. Sorry, too much going on.

Please can you merge cookie into your branch for this PR so we can see if tests are stlil passing.

Also, please can you show me the flake8 errors that caused you to make the RealMenu changes? This may change the behavior of the code, so we need to be super-sure on this. It seems to be beyond linting somehow.
Screenshot 2024-07-29 at 10 44 11 AM

@sbillinge heres a screenshot of the flake8 errors. Also, I am working on creating new PRs for the flake8 edits. Pt1, pt2, and pt3 of these PRs are open now.

@cadenmyers13 cadenmyers13 mentioned this pull request Jul 29, 2024
@cadenmyers13 cadenmyers13 deleted the flake8v4 branch July 31, 2024 16:20
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