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

daf_persistence: Tickets/dm 10340 #61

Merged
merged 15 commits into from Jun 27, 2017
Merged

daf_persistence: Tickets/dm 10340 #61

merged 15 commits into from Jun 27, 2017

Conversation

n8pease
Copy link
Contributor

@n8pease n8pease commented Jun 21, 2017

No description provided.

Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

Looks OK to me but I cannot say I understood completely how it works. Please check comments

The Repository class instance.

parentRepoDatas : list of RepoData
The are parents of this Repository, as indicated this Repository's RepositoryCfg. If this is a new
Copy link
Contributor

Choose a reason for hiding this comment

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

"The parents"?

repository, i.e. even though there is not a RepositoryCfg file, one will not be generated.
If False, this is a New Butler repository and is specified by RepositoryCfg file.

tags : set
Copy link
Contributor

Choose a reason for hiding this comment

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

set of what?

"parentRepoDatas={},\n\t" +
"isV1Repository={},\n\t"
"role={},\n\t" +
"parentRegistry={})\n\t").format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need trailing \n\t?

origin : string
'new', 'existing', or 'nested'
root : string
URI or absolute path to the location of the RepositoryCfg.yaml file.
Copy link
Contributor

Choose a reason for hiding this comment

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

isV1Repository is not documented


def getParentRepoDatas(self, context=None):
"""Get the parents & grandparents etc of this repo data, in depth-first search order.

Copy link
Contributor

Choose a reason for hiding this comment

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

context is undocumeted

if repoData.cfg.parents is None:
repoDataIdx += 1
continue # if there are no parents then there's nothing to do.
for repoParentIdx in range(len(repoData.cfg.parents)):
Copy link
Contributor

Choose a reason for hiding this comment

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

enumerate is probably more idiomatic

# if repoData is new, add the parent RepositoryCfgs to it.
if repoData.cfgOrigin == 'new':
repoData.cfg.addParents(parents)
elif repoData.cfgOrigin == 'existing' or repoData.cfgOrigin == 'nested':
Copy link
Contributor

Choose a reason for hiding this comment

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

repoData.cfgOrigin in ('existing', 'nested')?

@@ -0,0 +1,124 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not an executable script, no need for shebang


Parameters
----------
newParents : string or RepoistoryCfg instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a list of strings?


@dirty.setter
def dirty(self, val):
self._dirty = val
Copy link
Contributor

Choose a reason for hiding this comment

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

Property does not do anything special, could just use attribute?

Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

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

This review is for commit 76d7dc (safeFileIo.py). The logic there feels rather complicated, needed some verbal explanation for a dumb programmer like me. I think there is a benefit in making that logic simpler by not adding too much protection (avoiding writes to temporary file).

log.debug("Acquiring blocking shared lock on {}", name)
fcntl.flock(f, fcntl.LOCK_SH)
else:
raise e
Copy link
Contributor

Choose a reason for hiding this comment

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

raise without object is usually used for re-raising exception

@n8pease n8pease merged commit 9ff310b into master Jun 27, 2017
@ktlim ktlim deleted the tickets/DM-10340 branch August 25, 2018 06:14
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

2 participants