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

Remove deprecated coveragerc file #923

Merged
merged 8 commits into from
Oct 6, 2022

Conversation

opotowsky
Copy link
Member

@opotowsky opotowsky commented Oct 5, 2022

Description

There was a stale copy of a coverage config file in armi/resources. It's existence was only dreamed up to prevent the issues with dot-files on a Windows cluster. I changed the coverage work in case.py to copy the main coverage config file to a file name without a dot instead. Voila! No more forgetting to maintain multiple files.


Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@opotowsky opotowsky linked an issue Oct 5, 2022 that may be closed by this pull request
armi/cases/case.py Outdated Show resolved Hide resolved
@opotowsky
Copy link
Member Author

@john-science My history is a little mucked up from the interactive rebase. Working on fixing

@opotowsky opotowsky added the cleanup Code/comment cleanup: Low Priority label Oct 5, 2022
@opotowsky
Copy link
Member Author

Hey @john-science What happened with CI? Is there a way to kick it back off without making, e.g., an empty commit?

@jakehader
Copy link
Member

Hey @john-science What happened with CI? Is there a way to kick it back off without making, e.g., an empty commit?

I think making an empty / trivial commit is the only way that I can see to easily kick off the CI again for GitHub. There isn't an easy "rerun all" since we have different GitHub actions that get fired off in parallel. That is my recommendation, but maybe @john-science has a more elegant solution.

@opotowsky
Copy link
Member Author

The coverage drop would be expected, since I added coverage-related code that can't be unit tested.

Comment on lines 451 to 453
covFile = os.path.join(covRcDir, "coveragerc")
else:
covFile = os.path.join(covRcDir, ".coveragerc")
Copy link
Member

Choose a reason for hiding this comment

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

It looks to me like you could make this simpler:

if platform...
    if makeCopy...
        # Make ...

covFile = os.path.join(covRcDir, ".coveragerc")

It just looks like that else is unnecessary, and you could have the covFile = line once instead of twice. Right?

Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

I love removing the old cruft in the system!

Thanks!

@opotowsky opotowsky merged commit b4e477c into terrapower:main Oct 6, 2022
@opotowsky opotowsky deleted the deprecated_file branch October 6, 2022 16:14
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
* Remove extra covrc file and update case.py to handle the active file

* Fix _getCovRcFile to only make copy for cov.start

* Add test

* Release notes

* Add PROJECT_ROOT variable in context.py

* Address reviewer comment, sort of, with better if statement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused file
3 participants