Re: [orientdb] Code review questions

['scott molinari' via 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

-- 

--- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [orientdb] Code review questions

[Luca Garulli]

On 14 October 2016 at 17:12, Shahim Essaid  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:
>
> https://github.com/orientechnologies/orientdb/blob/
> b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/
> com/orientechnologies/orient/core/db/document/
> ODatabaseDocumentTx.java#L1348
>
> as seen for the similar method here:
>
> https://github.com/orientechnologies/orientdb/blob/
> b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/
> com/orientechnologies/orient/core/db/document/
> ODatabaseDocumentTx.java#L1365
>

You're right it's missing.


> 2. Database listeners are sometimes called for onBeforeTxBegin before the
> actual transaction object is set for the database. Is this intentional?
> Examples:
>
> https://github.com/orientechnologies/orientdb/blob/
> b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/
> com/orientechnologies/orient/core/db/document/
> ODatabaseDocumentTx.java#L1736
>
> https://github.com/orientechnologies/orientdb/blob/
> b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/
> com/orientechnologies/orient/core/db/document/
> ODatabaseDocumentTx.java#L2284
>

It's intentional, also the method's name onBeforeTxBegin() tells it's
called before the transaction begins.


> 3. DRY
>
> https://github.com/orientechnologies/orientdb/blob/
> b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/
> com/orientechnologies/orient/core/db/document/
> ODatabaseDocumentTx.java#L2341
>
> 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
OrientDB LTD 

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+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

-- 

--- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[orientdb] Code review questions

[Shahim Essaid]

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: 

https://github.com/orientechnologies/orientdb/blob/b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/com/orientechnologies/orient/core/db/document/ODatabaseDocumentTx.java#L1348

as seen for the similar method here:

https://github.com/orientechnologies/orientdb/blob/b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/com/orientechnologies/orient/core/db/document/ODatabaseDocumentTx.java#L1365

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

https://github.com/orientechnologies/orientdb/blob/b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/com/orientechnologies/orient/core/db/document/ODatabaseDocumentTx.java#L1736

https://github.com/orientechnologies/orientdb/blob/b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/com/orientechnologies/orient/core/db/document/ODatabaseDocumentTx.java#L2284

3. DRY

https://github.com/orientechnologies/orientdb/blob/b431805894b2599c3c7b4f066d31800ca504b389/core/src/main/java/com/orientechnologies/orient/core/db/document/ODatabaseDocumentTx.java#L2341

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

-- 

--- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.