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

Function and documentation updates #64

Closed
wants to merge 24 commits into from

Conversation

ChristensenCode
Copy link
Contributor

@ChristensenCode ChristensenCode commented Dec 9, 2022

Part 1 - December 6, 2022
The logic for the existing units class looked a little convoluted and I thought it might nicer to have it as a class. This way we can store all the conversions and the method names clearly define how the unit is changing. In addition, we can store common conversions in here like F to C, which is used in a few places in the code.

I updated the units tests to cover the new methods, but replacing the existing Units class look like a big task, so that will be done later.

I also started making a constants file for storing constants. The constants can then be defined in one place and referenced where they originated from.

Part 2 - December 10, 2022
Started updating the documentation to capture what data should be in the instruction set json file.
Added the sample json file from the documentation to give new users a starting point.

@ChristensenCode ChristensenCode changed the title Refactoring the units file and created a constants file Function and documentation updates Dec 10, 2022
@ChristensenCode
Copy link
Contributor Author

ChristensenCode commented Dec 12, 2022

I started looking into what's being done with the eval statements and they are unique.
Eval statements should be avoided for security reasons and the code is a little more tangled than I thought. So, it will take more time for me to tease out how to eliminate using inspect.getfullargspec and eval.

@lymereJ
Copy link
Collaborator

lymereJ commented Dec 12, 2022

@ChristensenCode - Thank you for your work! These look like great improvements. I have a few questions/comments:

  • Could you please move the replacement of print by logging to a separate PR? There's an open issue for it related to our submission to the JOSS. It'll be easier to keep track of the changes that way.
  • We're using black to format the code. I noticed that the GitHub actions trip on the black formatting checks. Could you please run black when you're done with the proposed changes.
  • Are you at PNNL? If so, could you please reach out to me on Teams :) ?

@ChristensenCode
Copy link
Contributor Author

@lymereJ
Yeah I can make the logging changes a different pull request since that's easier.

I'll run black on the files and update the contribution docs to reference using black when making contributions.

I don't work at PNNL, but I would like to. I figured jumping in on some open source work might be a good place to start.

This probably won't happen until this evening and I'm on east coast time.

@ChristensenCode
Copy link
Contributor Author

the pull request for units and doc changes and replaced the print statements replace this request so I can close this one.

Since I cherry-picked the logging changes for that pull request, there might be some conflicts with the units and docs changes.

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