cleanup euids #35
cleanup euids #35
Conversation
There was a problem hiding this comment.
Looks good, but two general comments:
- Inconsistent use of EUID vs. Euid. Not sure which is correct according to Java naming conventions.
- Would it make sense to change
toCedartotoString? I think this would make it a little more obvious what the code is doing. Also: why are there different implementations fortoCedarandtoString? When would you wantUser::aliceinstead ofUser::"alice"?
| actionEuid = "Action::\"" + actionId + "\""; | ||
| if (!e.getEuid().toCedar().equals(actionEuid)) { | ||
| e.parentsEUIDs.add(new JsonEUID(actionType, actionId)); |
There was a problem hiding this comment.
Seems like it would be a little cleaner to test equality of e and the action type without converting to a string. So save new JsonEUID(actionType, actionId) to some variable uid and then check!e.getEuid().equals(uid)
There was a problem hiding this comment.
We can't use the default equals method here and if I define equals SpotBugs says I should also define hashCode. Since I don't want to do that and string equality is exactly what we want, I think I'll leave as-is. I did rename toCedar -> toString and removed the old toString though.
There was a problem hiding this comment.
Sounds good. What about the EUID vs. Euid issue?
There was a problem hiding this comment.
Let's change that in a separate PR since it will touch a bunch of files.
There was a problem hiding this comment.
I found https://stackoverflow.com/questions/15526107/acronyms-in-camelcase which gives various options. I personally found this answer (https://stackoverflow.com/a/45440841) most reasonable. Unfortunately, it violates Jackson's naming convention (JavaScript Object Notation could be argued to go to JSON or JsON but not Json). It would also indicate Entity Unique Identifier should go to EUId which 1) looks weird to me and 2) doesn't match our docs.
I put up (#42) with some obvious consistency changes, but I'm not convinced which style is best for us, so it's only Euid -> EUID
|
Heads up: I just merged #39 , which may cause conflicts with this PR |
Issue #, if available:
Description of changes:
Cleanup code from #33 Change Entities to always store the EUID as a JsonEUID object instead of sometimes storing this and sometimes a String representation. Make the same change for
Entities.parents.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.