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

Update config file logic, tests #201

Merged
merged 37 commits into from
Nov 26, 2019
Merged

Update config file logic, tests #201

merged 37 commits into from
Nov 26, 2019

Conversation

mjn0898
Copy link
Contributor

@mjn0898 mjn0898 commented Nov 23, 2019

Allowing config file object to be passed into init

allow config file object to be passed into __init__
Copy link
Member

@lewismc lewismc left a comment

Choose a reason for hiding this comment

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

Please address comments
Also, please provide unit tests for the following

  • incorrect paths
  • garbled, garbage config values
    Thanks

@mjn0898
Copy link
Contributor Author

mjn0898 commented Nov 23, 2019

The tests are in progress, I just didn't have time to do them today.
I am a bit confused on wanting that logic in the class invocation because you wanted the config file to be passed in as a parameter to the init function, which is why I now pass in a file object which I have to create elsewhere.

@lewismc
Copy link
Member

lewismc commented Nov 23, 2019

The tests are in progress, I just didn't have time to do them today.

No problem. That's fine. I understand.

I am a bit confused on wanting that logic in the class invocation because you wanted the config file to be passed in as a parameter to the init function, which is why I now pass in a file object which I have to create elsewhere.

Here's what I think would make a sensible API

  1. augment the pycoal-mineral CLI options to include an option for a configuration file,
  2. augment the MineralClassification init function such that, again, it includes an option for a configuration file. In this case the os.path.join(os.path.dirname(__file__)... logic should reside within MineralClassification __init__ because it may not be called from the example, it may be called from somewhere else e.g. another Python script or pycoal-mineral CLI.

Does that make sense?

@lewismc
Copy link
Member

lewismc commented Nov 23, 2019

Also, please always label your issues correctly and assign them to the current project. Thanks @mjn0898 good work.

@mjn0898 mjn0898 added this to In progress in USC Capstone '19 via automation Nov 23, 2019
@mjn0898 mjn0898 self-assigned this Nov 23, 2019
Copy link
Member

@lewismc lewismc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, please see my comments.

pycoal/cli/mineral.py Outdated Show resolved Hide resolved
pycoal/mineral.py Outdated Show resolved Hide resolved
USC Capstone '19 automation moved this from In progress to Review in progress Nov 25, 2019
@lewismc
Copy link
Member

lewismc commented Nov 25, 2019

Also, @mjn0898 please address the DeepSource issues. Thanks

@mjn0898
Copy link
Contributor Author

mjn0898 commented Nov 25, 2019

I fixed the deepsource issues. As for the build errors due to some new tests failing, most of the errors are due to the new algorithm functions not being up to date on this branch.

@lewismc
Copy link
Member

lewismc commented Nov 25, 2019

@mjn0898 this is looking much better.

As for the build errors due to some new tests failing, most of the errors are due to the new algorithm functions not being up to date on this branch.

Well then please just use the following annotation for the time being

@unittest.skip("reason for skipping")

See the follosing SO post - https://stackoverflow.com/questions/2066508/disable-individual-python-unit-tests-temporarily
Thanks

@lewismc lewismc added this to the 0.6 milestone Nov 26, 2019
@lewismc lewismc merged commit 743291d into master Nov 26, 2019
USC Capstone '19 automation moved this from Review in progress to Done Nov 26, 2019
@lewismc lewismc deleted the ISSUE-196-config_path branch November 26, 2019 04:58
@lewismc
Copy link
Member

lewismc commented Nov 26, 2019

Thank you @mjn0898 good work on this branch. Some notes for your consideration

  • please always create an issue before you create the pull request. This should describe the scope of the work. Your commit messages should also reflect this scope.
  • on commit messages, please see the following - https://chris.beams.io/posts/git-commit/

Thank you, good work

@mjn0898
Copy link
Contributor Author

mjn0898 commented Nov 26, 2019

Thanks, I will keep that in mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants