style: remove unused module, change style of string, fix codespell. #15
Conversation
src/diffpy/confutils/__init__.py
Outdated
|
|
||
| # some convenience imports | ||
| from diffpy.confutils.config import ConfigBase | ||
| from diffpy.confutils.config import ConfigBase # noqa: F401 |
There was a problem hiding this comment.
Did you # noqa these lines because of another pre-commit conflict error?
There was a problem hiding this comment.
No, it is because I'm worrying about whether deleting this module would affect the base functionality for the package. So, I put it here to silence for flake8 for now.
There was a problem hiding this comment.
Hmm, after looking at the package again, I'm not quite sure what this confutils folder is for. It seems like a subpackage, but the code in this folder seems to just be taken from the main diffpy.srxconfutils package that was recently migrated. Maybe it's actually fine to delete this folder altogether? I'm not sure, so let's ask Simon to review
There was a problem hiding this comment.
@sbillinge Professor we are not sure whether we need to keep this?
There was a problem hiding this comment.
it would be good to delete it if we can, but we would like to either verify that it doesn't break anything, or else verify that the code you are deleting is the same as what it is replaced with (or both).
There was a problem hiding this comment.
@zmx27 Would it be possible to check it by pulling it into the GUI test if I delete it? It it could, I would delete it and we could check if we would break anything.
There was a problem hiding this comment.
it would be good to delete it if we can, but we would like to either verify that it doesn't break anything, or else verify that the code you are deleting is the same as what it is replaced with (or both).
Yes, I do believe that the code in the confutils folder is taken directly from the srxconfutils package. @stevenhua0320 let's delete this folder and I'll get back to you on whether or not anything breaks from doing that.
|
|
||
| # package version | ||
| from diffpy.srxplanar.version import __version__ | ||
| from diffpy.srxplanar.version import __version__ # noqa: F401 |
There was a problem hiding this comment.
Same here, why are you putting so many # noqa here? I'm not totally sure, but I think that skpkg has a way of silencing pre-commit errors that may be introduced when certain imports are unused
There was a problem hiding this comment.
I silent it here because now we are not at the stage of real migration, these imports are necessary after we migrate this file into right folder. So, I silence it for now.
There was a problem hiding this comment.
What do you mean by migrating into the right folder?
There was a problem hiding this comment.
I mean similar to diffpy.srxplanargui's situation, here we are not at the stage of using these imports? But when we have done the migration this module import might become useful? I'm not very sure for that.
There was a problem hiding this comment.
You're right, this file doesn't matter too much at this point. We can probably keep this and wait for the skpkging
There was a problem hiding this comment.
it is preferred not to noqa things that will be replaced. it is ok for pre-commit to fail on these as they will be changed later by the packaging step.
|
@zmx27 I have deleted the |
|
I added an issue to revisit later and try and resolve the noqa's after we do the scikit package. With this done, I am happy to merge PRs with many noqa's if we think they may go away during the packaging process as we remove py2 things and other stuff. That said, this PR is now conflicted.... |
It conflicted because now I deleted the |
This reverts commit 105d4cd.
|
@sbillinge I have seen the issue you set there, after considering it, I revert the deletion of the |
3 participants
@zmx27 Ready for review