Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removed unused .gitignore files #98

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

mkhansenbot
Copy link
Collaborator

Fixes issue #56
Removes unused .gitignore files

@mkhansenbot mkhansenbot added this to the September 2018 milestone Sep 25, 2018
@mkhansenbot mkhansenbot self-assigned this Sep 25, 2018
@mkhansenbot mkhansenbot added this to In progress in Navigation 2 Kanban via automation Sep 25, 2018
Navigation 2 Kanban automation moved this from In progress to Reviewer approved Sep 26, 2018
Copy link

@mjeronimo mjeronimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were just placeholders to demonstrate the directory hierarchy. I'd rather keep them until the directories are populated, at which point they can be deleted.

@mkhansenbot
Copy link
Collaborator Author

We aren't going to need these gitignore files since we have one at the top level. I don't think we should have empty directories. Removing them will remove confusion for users coming to our repo. If they have to sift through a bunch of empty directories that will confuse them and waste their time. The overall heirarchy is being maintained here, just not empty files / directories. All the top level folders are still there.

@mjeronimo
Copy link

mjeronimo commented Sep 26, 2018

No, we aren't going to need these as .gitignore files. They are just there so that a file is present (and hence the directory shows up in the hiearchy). If you like, we can use a different filename, such as .TODO or something. The files are present to encourange the correct directory structure as the hiearchy is filled out. Let's discuss, if necessary.

@mkhansenbot
Copy link
Collaborator Author

Take a look at my branch here: https://github.com/mkhansen-intel/navigation2/tree/rm_gitignore/src

You'll see that all the main folders are still there (navigation, planning, etc.) We're just pruning the unpopulated leaf folders. There is still a clear hierarchy in place. Also, when people submit PRs for code review, we can ensure they put new code in the right place then. So I don't see a need for these placeholders anymore. They have served their purpose in my opinion, and they confuse new users.

@mjeronimo
Copy link

mjeronimo commented Sep 26, 2018

Yeah, I understand but disagree. The basic difference is: do we have a template that we fill out, or build incrementally. I prefer the former as it demonstrates the directories and files that will be populated. I believe that it limits confusion as well as the various message and test directories are already defined according to a uniform naming convention.

@mkhansenbot
Copy link
Collaborator Author

OK, just to document our stand-up discussion this morning, we agreed that we'll delete these files but add an outline of the directory structure we want to the contributing section of the documentation README file. Mike's going to make that change and push it to the branch as part of this PR.

@mjeronimo mjeronimo merged commit 373087b into ros-navigation:master Sep 28, 2018
Navigation 2 Kanban automation moved this from Reviewer approved to Done Sep 28, 2018
@mjeronimo
Copy link

The outline of the directory structure won't be needed not that we're going to reorganize according the Steve's PR, so I went ahead and integrated this and will close this issue.

@mkhansenbot mkhansenbot deleted the rm_gitignore branch November 1, 2018 19:25
ghost pushed a commit to logivations/navigation2 that referenced this pull request Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants