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.

Sign up for GitHub

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

Jump to bottom

Restructure project #12

Merged
merged 1 commit into from
Aug 4, 2017
Merged

Restructure project #12

merged 1 commit into from
Aug 4, 2017

Conversation

LauLaman
Copy link
Contributor

@LauLaman LauLaman commented Aug 3, 2017 •
edited

According to @holtkamp's issue #4 :

  • composer.lock should probably not be committed to repository and part of .gitignore
  • .idea folder should probably not be committed to repository and part of .gitignore
  • the lib folder should "probably" be renamed to src
    • I believe this is not a "hard" rule, more of a PSR4 coding convention... lib suggests 3rd party code, while src is "your" code..

dnl-blkv
dnl-blkv suggested changes Aug 4, 2017
View reviewed changes
Copy link
Contributor

@dnl-blkv dnl-blkv left a comment

Choose a reason for hiding this comment

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

.idea and composer.lock should stay. They are useful for developers, and we will exclude them from releases only, so please revert those removals.

lib to src is a valid change, please keep it :)

.gitignore Outdated
# Gradle:
.idea/**/gradle.xml
.idea/**/libraries
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

This will stay - we need it for development purposes. We will exclude it from releases though. So, please return it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats why i made atomic commits. 😎 give me a sec. i'll drop those commits

@LauLaman
Copy link
Contributor Author

LauLaman commented Aug 4, 2017

Done

@OGKevin OGKevin requested a review from dnl-blkv August 4, 2017 12:57
OGKevin
OGKevin approved these changes Aug 4, 2017
View reviewed changes
Copy link
Contributor

@OGKevin OGKevin left a comment

Choose a reason for hiding this comment

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

Tests passing @dnl-blkv

@dnl-blkv dnl-blkv merged commit 2d41e32 into bunq:master Aug 4, 2017
@dnl-blkv
Copy link
Contributor

dnl-blkv commented Aug 4, 2017 •
edited

Thanks @LauLaman and congratz with the first code contribution! 🥇

@LauLaman
Copy link
Contributor Author

LauLaman commented Aug 4, 2017 •
edited

@dnl-blkv Thanks! 🎉
By the way if you set te repo settings to only accept Rebase and Merge option. you'll lose the ugly (extra) merge commits:
screen shot 2017-08-04 at 15 19 29

You can set it here (look for Merge button)

@LauLaman LauLaman deleted the restructure_project branch August 4, 2017 14:20
@WouterFlorijn WouterFlorijn mentioned this pull request Jul 4, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnl-blkv dnl-blkv dnl-blkv approved these changes

@OGKevin OGKevin OGKevin approved these changes

Assignees

@LauLaman LauLaman

@OGKevin OGKevin

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LauLaman @dnl-blkv @OGKevin