Code review questions

62 views
Skip to first unread message

Shahim Essaid

unread,
Oct 14, 2016, 6:12:29 PM10/14/16
to OrientDB
Hi,

What is the best way to ask questions if I'm reviewing some code and I think I see a problem, or I need some clarification? I know GitHub has a commenting feature for pull requests and on blame views but there is no way to simply place comments on arbitrary lines in current files.  I also looked for an issue label like "code review" or something similar and I didn't notice one.

Examples of what I'm talking about are:

1. Why is there no checkSecurity here: 


as seen for the similar method here:


2. Database listeners are sometimes called for onBeforeTxBegin before the actual transaction object is set for the database. Is this intentional? Examples:



3. DRY


can simply be  this.freeze(false);

I'm starting to study some of the code and I'm sure I'll have many similar questions. Should these be issues? One issue per file? Which label should I use?

Best,
Shahim

scott molinari

unread,
Oct 17, 2016, 5:07:19 AM10/17/16
to OrientDB
I am not a developer, but I think you'd need to be doing any code checks on the develop branch. That is where I believe the party is at. :-)

Scott

Luca Garulli

unread,
Oct 17, 2016, 9:45:26 AM10/17/16
to OrientDB
On 14 October 2016 at 17:12, Shahim Essaid <sha...@essaid.com> wrote:
Hi,

Hi Shahim, 

What is the best way to ask questions if I'm reviewing some code and I think I see a problem, or I need some clarification? I know GitHub has a commenting feature for pull requests and on blame views but there is no way to simply place comments on arbitrary lines in current files.  I also looked for an issue label like "code review" or something similar and I didn't notice one.

Examples of what I'm talking about are:

1. Why is there no checkSecurity here: 


as seen for the similar method here:


You're right it's missing.
It's intentional, also the method's name onBeforeTxBegin() tells it's called before the transaction begins. 
 
3. DRY


can simply be  this.freeze(false);

I'm starting to study some of the code and I'm sure I'll have many similar questions. Should these be issues? One issue per file? Which label should I use?

You're right, it could call that methof.

WDYT about sending a Pull Request on GitHub against the develop branch? I'd be more than happy to accept it.


Best Regards,

Luca Garulli
Founder & CEO

Want to share your opinion about OrientDB?
Rate & review us at Gartner's Software Review

 

Best,
Shahim

--

---
You received this message because you are subscribed to the Google Groups "OrientDB" group.
To unsubscribe from this group and stop receiving emails from it, send an email to orient-database+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

scott molinari

unread,
Oct 18, 2016, 5:03:03 AM10/18/16
to OrientDB
Luca,

Does that mean the links given are in the develop branch? I didn't think they are, because I went to the same lines in develop and they looked different.

Scott
Reply all
Reply to author
Forward
0 new messages