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

DOC:cache.py/equilibrium.py/database.py/tdb.py:Edit docstrings for co… #104

Merged
merged 2 commits into from Aug 2, 2017
Merged

Conversation

ghost
Copy link

@ghost ghost commented Jun 29, 2017

…nsistent format

@ghost
Copy link
Author

ghost commented Jun 29, 2017

@bocklund

@bocklund
Copy link
Collaborator

It looks like the AppVeyor continuous integration build failed. Can you take a look at the Details for one of the builds and see if one of your changes caused the failure?

@bocklund
Copy link
Collaborator

@Olivia-Higgins anything I can help with on this?

@ghost
Copy link
Author

ghost commented Jul 10, 2017

I did what you said to do up to the forced push. I was getting error messages saying that I needed to set an upstream for my branch. I followed the recommendations the message gave and then continued with the forced push. Let me know if this was not the correct thing to do, thanks.

@bocklund
Copy link
Collaborator

Your changes pushed correctly, just waiting on the tests now.

On the setting upstream: the first time you pushed, you probably typed something like git push origin fix-typos then just now to force push maybe you only did git push -f? If that was the case, it would give you those error messages to set an upstream because it doesn't know where to push to. If you did the suggested git push -f -u origin fix-typos then the -u option set the upstream to the fix-typos branch and from now on git push or git push -f will correctly push to the correct branch.

@bocklund
Copy link
Collaborator

@Olivia-Higgins I don't think we are actually using the log.py file anywhere. Do you want to just remove it and push those changes and see if tests pass?

@ghost
Copy link
Author

ghost commented Jul 20, 2017

@bocklund This time there is an ImportError, the error message reads "no module named log". Any suggestions on what else to try?

@bocklund
Copy link
Collaborator

We are importing and using logging in a couple places it looks like: https://github.com/pycalphad/pycalphad/search?utf8=%E2%9C%93&q=logger&type=

I think the debug in model could just be removed, the logger.warning should be made into a real warning with the Python standard library warnings module, which is already imported in calculate.py.

@bocklund
Copy link
Collaborator

@Olivia-Higgins (via email): For changing the logger.warning into a warning, when using the warn() function is there a way to keep the message format as it is in logger.warning, so that phase_name is inserted into the printed message?

logging has the format string stuff built in (fmt = '%(name)s...' in line 9 of log.py). Now you'll have to format things manually instead of depending on the logger to do it for you. https://pyformat.info/ is a really good reference.

In pycalphad we mix some of the '%' (old) style strings with the .format() (new) style. Use the new style in all new code.

Copy link
Collaborator

@bocklund bocklund left a comment

Choose a reason for hiding this comment

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

Happy to see this passing! Merging this.

@bocklund bocklund merged commit 6ad41e6 into pycalphad:develop Aug 2, 2017
@ghost ghost deleted the fix-typos branch August 2, 2017 19:52
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.

None yet

1 participant