fixing to_datacie, allowing for None in roleName

[djarecka]

fixes #80

[codecov]

Codecov Report

Merging #81 (fef797c) into master (1054377) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #81   +/-   ##
=======================================
  Coverage   90.13%   90.13%           
=======================================
  Files          13       13           
  Lines        1318     1318           
=======================================
  Hits         1188     1188           
  Misses        130      130           

Flag	Coverage Δ
unittests	90.13% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dandischema/tests/test_datacite.py	43.66% <ø> (ø)
dandischema/datacite.py	56.12% <100.00%> (ø)

---

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 1054377...fef797c. Read the comment docs.

[yarikoptic]

what was rationale for this particular line change? getattr is safer and I might have been used for a reason (some prior version having no .roleName?) so I would have just kept this as is.

Otherwise looks good to me

[waxlamp]

getattr() raises AttributeError if the attribute doesn't exist (according to the hasattr() docs), so this change is actually better than calling getattr(), because it's a more standard expression of pulling an attribute from an object.

[djarecka]

I've changed it only because I realize that roleName has None as default, so the attribute should always exist, but I could revert

[waxlamp]

As I mentioned, I'd keep this the way it is now, since the old and new versions have exactly the same effect, but the new version is more standard.

[yarikoptic]

Thank you @waxlamp ! Indeed, this RF is ok here (although unrelated to the purpose of the PR). I guess while analyzing for it, I have also misguided myself by the fact that dict.get(key) does default to not blowing up but just returning None if key is not in the dict. May be original intention was similarly misguided in 4878d23 @djarecka but lacked that specification of the default?

[djarecka]

@yarikoptic - the only reason why I've changes it here was because I was exploring the default behavior if nothing is passed as roleName (that was the example that caused an issue for you).

[djarecka]

@waxlamp - yes, that's true that getattr returns None, so it's often a save check that avoids raising any exceptions. Probably that was the reason why I used it at the beginning.

[yarikoptic]

@waxlamp - yes, that's true that getattr returns None, so it's often a save check that avoids raising any exceptions. Probably that was the reason why I used it at the beginning.

may be a typo (dict.get instead of getattr?) since that is the point @waxlamp mentioned above that getattr does raise exception if the default is not specified explicitly (which differs in behavior from dict.get where default does not need to be specified and defaults to None)

[yarikoptic]

overall -- there is no point to use getattr(obj, "string with the fieldname") over obj.fieldname, unless providing an explicit default, as e.g. getattr(obj, "string with the fieldname", None)

[djarecka]

yes, I'm sorry. I've mixed also in the discussion getattr and dict.get. But I understand that this could stay like this

[yarikoptic]

anyways , for the purpose of a fix I think we have reviewed it and there is even a test -- nothing should hold the progress. to facilitate immediate availability of the fix for dandi-api I will add 'release' label and merge. Current git describe: 0.3.2-3-g1054377 -- with the release will test new versiongit ;-)