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

Add idf.copy and idf.deepcopy methods and tests #124

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jamiebull1
Copy link
Collaborator

Implements #118

@codecov-io
Copy link

codecov-io commented Sep 7, 2016

Codecov Report

❗ No coverage uploaded for pull request base (develop@6b1f7b9). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             develop    #124   +/-   ##
=========================================
  Coverage           ?   41.9%           
=========================================
  Files              ?      52           
  Lines              ?    5758           
  Branches           ?       0           
=========================================
  Hits               ?    2413           
  Misses             ?    3345           
  Partials           ?       0
Impacted Files Coverage Δ
eppy/modeleditor.py 94.54% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b1f7b9...c4d2c83. Read the comment docs.

@santoshphilip
Copy link
Owner

santoshphilip commented Sep 8, 2016

Thoughts on API

Many of the users of eppy are not programmers. They write scripts to get something done. The API should read meaningfully to them.
idf.deepcopy may not make sense to them
idf.copy should probably be the name for deepcopy

should there be a idf.shallowcopy ? Can you think of a use case for this ?

PS: I have not actually read your code yet. I am going off your comments and code in #118

API design driven by documentation and tutorial

Continuing to think aloud here. My touchstone is to pretend I am writing a tutorial for this with the end user in mind. If it takes a lot to explain, the API design needs to be revisited.
In fact when I wrote the tutorial for eppy, I wound up rewriting the code, since I had a hard time explaining how to use it :-)

Zen of eppy for developers

  • code should be written to make the tutorial easy to write.

@santoshphilip
Copy link
Owner

I'll go ahead and

  • tweak the API
  • update documentation
  • merge into develop

@jamiebull1
Copy link
Collaborator Author

I totally agree with you on how writing documentation helps you rethink the API. I did the same thing when writing some docs for GeomEppy earlier this week - it's also why I like to document all my functions and methods well.

My problems with your suggestion that IDF.copy should be the name for IDF.deepcopy and that we remove IDF.(shallow)copy are that a) it runs counter to what an experienced Python developer would expect and b) it leaves us in a situation where idf.copy() and copy(idf) do different things. This is an area, like with IDF.printidf() where there is a native Python way to do things (print(IDF)) and an Eppy way to do things which runs slightly counter to the Zen of Python, but worse here if they don't do the same thing. I think the only reason we're justified having it at all in this case is that deepcopy(idf) fails on Python. Perhaps a better course of action is to figure out why deepcopy is trying to operate on a closed file in Python 3 and fix that instead.

I'm happy to have a crack at that, but I'm away now till Sunday night so will have to wait.

@santoshphilip
Copy link
Owner

I am moving to the conclusion that python's deepcopy is the wrong function for this.
IDF has the idd file embedded in it. And snippets of the idd are scattered through the IDF.

In eppy there is only a single copy of the idd.

The idd and elements of the idd should not be copied. Python's deepcopy does not have the intelligence to do this.

The most reliable way of making a copy of the IDF would be to save the idf to a StringIO (or file on disk) and open it again from the StringIO (or file on disk). It does not feel very elegant, but is rather pragmatic.

@jamiebull1
Copy link
Collaborator Author

Ok, that makes sense, and is what the code I submitted does for python 3,
just reading idf.idfstr() as a new IDF. It's not a common action in any
case so efficiency isn't hugely important.

It's probably worth making a note that while deepcopy might work, its
behaviour isn't guaranteed. Is there a sensible place in the docs for that?

On Sun, 11 Sep 2016, 07:32 santoshphilip, notifications@github.com wrote:

I am moving to the conclusion that python's deepcopy is the wrong function
for this.
IDF has the idd file embedded in it. And snippets of the idd are scattered
through the IDF.

In eppy there is only a single copy of the idd.

The idd and elements of the idd should not be copied. Python's deepcopy
does not have the intelligence to do this.

The most reliable way of making a copy of the IDF would be to save the idf
to a StringIO (or file on disk) and open it again from the StringIO (or
file on disk). It does not feel very elegant, but is rather pragmatic.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#124 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADo-yIZzvxLCfZe_IVmfhF4maZoGttm4ks5qo5JpgaJpZM4J3Zrn
.

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.

3 participants