GNOME Bugzilla – Bug 772316
create new function to check explicit regular file
Last modified: 2016-10-05 16:54:31 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.
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.
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.
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
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.
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.
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
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.
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"
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.
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.
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.
Review of attachment 336989 [details] [review]: Looks good now! Thanks and congrats!!
Pushed Attachment 336989 [details] pushed as dd78529 - file: add function nautilus_file_is_regular_file