After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772316 - create new function to check explicit regular file
create new function to check explicit regular file
Status: RESOLVED FIXED
Product: nautilus
Classification: Core
Component: general
3.21.x
Other Linux
: Normal normal
: ---
Assigned To: Nautilus Maintainers
Nautilus Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-10-01 23:16 UTC by Neha Yadav
Modified: 2016-10-05 16:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add function nautilus_file_is_regular_file (2.05 KB, patch)
2016-10-01 23:16 UTC, Neha Yadav
none Details | Review
Add function nautilus_file_is_regular_file (1.50 KB, patch)
2016-10-03 09:33 UTC, Neha Yadav
none Details | Review
Add function nautilus_file_is_regular_file (2.15 KB, patch)
2016-10-04 21:53 UTC, Neha Yadav
none Details | Review
Add function nautilus_file_is_regular_file (2.46 KB, patch)
2016-10-05 10:42 UTC, Neha Yadav
reviewed Details | Review
file: add function nautilus_file_is_regular_file (2.47 KB, patch)
2016-10-05 14:07 UTC, Neha Yadav
none Details | Review
file: add function nautilus_file_is_regular_file (2.47 KB, patch)
2016-10-05 14:44 UTC, Neha Yadav
committed Details | Review

Description Neha Yadav 2016-10-01 23:16:44 UTC
Created attachment 336741 [details] [review]
Add function nautilus_file_is_regular_file

Any other person reading the code cannot know wheter there
    are only two options, so I can imagine we could add a
    function nautilus_file_is_regular_file() and check
    for that instead.
Comment 1 Carlos Soriano 2016-10-03 08:35:35 UTC
Review of attachment 336741 [details] [review]:

Hey Neha, thanks for the patch!

The commit message title is missing the file that is being touched. Also don't use caps in the first character of the title.

This part of the commit message is written in the wrong tense and voice:
so I can imagine we could add a
function nautilus_file_is_regular_file() and check
for that instead.

Instead it should be:
This patch adds a new function nautilus_file_is_regular_file so we can check for explicit regular file type.

::: src/nautilus-file.h
@@ +207,3 @@
 gboolean                nautilus_file_is_executable                     (NautilusFile                   *file);
 gboolean                nautilus_file_is_directory                      (NautilusFile                   *file);
+gboolean                nautilus_file_is_regular_file					(NautilusFile                   *file);

No tabs, use spaces.
Comment 2 Neha Yadav 2016-10-03 09:33:24 UTC
Created attachment 336798 [details] [review]
Add function nautilus_file_is_regular_file

This patch adds a new function
nautilus_file_is_regular_file so we
can check for explicit regular file type.
Comment 3 Carlos Soriano 2016-10-04 20:32:55 UTC
Review of attachment 336798 [details] [review]:

Hey Neha, another review round!

This patch is a patch above another patch. As similar as I pointed out in the other bug report, read https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Follow_Up_on_the_Feedback to know how to follow up on feedback.
Also, the commit message doesn't follow the guidelines in https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines
I has these problems:
- The line with is too short
- It lacks the name of the file you are touching in the title
- It doesn't explain why we need this new function
Comment 4 Neha Yadav 2016-10-04 21:53:36 UTC
Created attachment 336957 [details] [review]
Add function nautilus_file_is_regular_file

Create nautilus_file_is_regular_file instead of using opposite
function "!nautilus_file_is_directory()"

Any other person reading the code cannot know wheter there is
any other option in place of "!nautilus_file_is_directory()"
is available or not.

To fix this issue add a new function nautilus_file_is_regular_file
inside nautilus-file. So, we can check for explicit regular file type.
Comment 5 Carlos Soriano 2016-10-05 09:30:43 UTC
Review of attachment 336957 [details] [review]:

Patch looking better except:

::: src/nautilus-file.h
@@ +207,3 @@
 gboolean                nautilus_file_is_executable                     (NautilusFile                   *file);
 gboolean                nautilus_file_is_directory                      (NautilusFile                   *file);
+gboolean                nautilus_file_is_regular_file					(NautilusFile                   *file);

I already mentioned this... don't use tabs but spaces and align it with the previous functions.
Comment 6 Carlos Soriano 2016-10-05 09:31:47 UTC
Also the commit message doesn't have the file the patch is touching in the commit message tittle. Read https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines to know how to make a correct commit message
Comment 7 Neha Yadav 2016-10-05 10:42:27 UTC
Created attachment 336972 [details] [review]
Add function nautilus_file_is_regular_file

In commit we added a new function named nautilus_file_is_regular_file

The problem is that there are function present in different files
named "!nautilus_file_is_directory()" like inside nautilus-file.c
near line #7798. So, if any newcomer will come and look into the files
then it is difficult for them to analyse that there is any other
option available for file or not.

To solve this issue create a new function inside nautilus-file
with name nautilus_file_is_a_regular_file. So, we can check
for explicit regular file type.
Comment 8 Carlos Soriano 2016-10-05 13:43:10 UTC
Review of attachment 336972 [details] [review]:

The commit message is still missing the file the patch touchs in the tittle. It should be something like:
"file: add function nautilus_file_is_regular_file"
Comment 9 Neha Yadav 2016-10-05 14:07:37 UTC
Created attachment 336987 [details] [review]
file: add function nautilus_file_is_regular_file

In commit we added a new function named nautilus_file_is_regular_file

The problem is that there are function present in different files
named "!nautilus_file_is_directory()" like inside nautilus-file.c
near line #7798. So, if any newcomer will come and look into the files
then it is difficult for them to analyse that there is any other
option available for file or not.

To solve this issue create a new function inside nautilus-file
with name nautilus_file_is_a_regular_file. So, we can check
for explicit regular file type.
Comment 10 Carlos Soriano 2016-10-05 14:28:00 UTC
Review of attachment 336987 [details] [review]:

Looking much better! Only thing left:

::: src/nautilus-file.c
@@ +8070,3 @@
+ *@file: NautilusFile representing the file in question.
+ *
+ *Returns: TRUE if @file is a regular file.

Nice to have a comment here!
As the other comments though you need a space after every * that has some sentence afterwards.
Comment 11 Neha Yadav 2016-10-05 14:44:47 UTC
Created attachment 336989 [details] [review]
file: add function nautilus_file_is_regular_file

In commit we added a new function named nautilus_file_is_regular_file

The problem is that there are function present in different files
named "!nautilus_file_is_directory()" like inside nautilus-file.c
near line #7798. So, if any newcomer will come and look into the files
then it is difficult for them to analyse that there is any other
option available for file or not.

To solve this issue create a new function inside nautilus-file
with name nautilus_file_is_a_regular_file. So, we can check
for explicit regular file type.
Comment 12 Carlos Soriano 2016-10-05 16:22:06 UTC
Review of attachment 336989 [details] [review]:

Looks good now! Thanks and congrats!!
Comment 13 Carlos Soriano 2016-10-05 16:54:27 UTC
Pushed

Attachment 336989 [details] pushed as dd78529 - file: add function nautilus_file_is_regular_file