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

Move application code to "src" directory #140

Closed
wants to merge 1 commit into from

Conversation

zhukovra
Copy link

Q A
Is bugfix? no
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues

It is better to organize application code under a separate directory like src. Also, it is a widely used convention.

@zhukovra
Copy link
Author

@klimov-paul what do you think about it?

@samdark
Copy link
Member

samdark commented Jan 30, 2018

It's OK for new repositories but doesn't make much sense for existing ones.

@samdark
Copy link
Member

samdark commented Jan 30, 2018

Shouldn't break anything though so if @klimov-paul is OK with the change, I see no problem making it.

@zhukovra
Copy link
Author

@samdark if you want to whitelisting code coverage in phpunit.dist.xml @ 9ef0dbd:

<filter>
  <whitelist processUncoveredFilesFromWhitelist="true">
    <directory suffix=".php">.</directory>
    <exclude>
      <directory>.github/</directory>
      <directory>debug/</directory>
      <directory>docs/</directory>
      <directory>tests/</directory>
      <directory>vendor/</directory>
    </exclude>
  </whitelist>
</filter>

and after that PR:

<filter>
  <whitelist processUncoveredFilesFromWhitelist="true">
    <directory suffix=".php">src/</directory>
  </whitelist>
</filter>

@klimov-paul
Copy link
Member

This matter is already been discussed at yiisoft/yii2#8452 (comment)

Change can be applied. However there should be consistency between different repositories.
Either all extensions should be switched to this layout or none of them.

@rob006
Copy link

rob006 commented Jan 30, 2018

New extensions (queue, di) uses this template, so it's already inconsistent.

@zhukovra
Copy link
Author

zhukovra commented Jan 30, 2018 via email

@samdark
Copy link
Member

samdark commented Jan 30, 2018

Let's switch. It's making everything a bit more structured.

@zhukovra
Copy link
Author

@klimov-paul an elephant is eaten in parts. Let's do it 😃

@zhukovra
Copy link
Author

@samdark @klimov-paul hi, guys. I've created 2 PR in yii2-redis and yii2-sphinx extensions. Will I continue on switching repositories to "src" template?

@klimov-paul klimov-paul self-assigned this Jan 31, 2018
@klimov-paul klimov-paul added the type:enhancement Enhancement label Jan 31, 2018
@klimov-paul klimov-paul added this to the 2.0.6 milestone Jan 31, 2018
@klimov-paul
Copy link
Member

Resolved by commit 7ccba47

@zhukovra
Copy link
Author

@klimov-paul can you tell me what kind of opensource development methodology are you using? I mean situation when you close my PRs

with cherry-picked commits from my branches authored by you. At least it looks like that.

@samdark @cebe @qiangxue is it normal? I've never seen it on Github.

@samdark
Copy link
Member

samdark commented Jan 31, 2018

@zhukovra Of course, you should've been attributed. I think Paul did am and forgot to add --author when committing. @klimov-paul right?

Since commits are in master, I don't think we can update them without changing hashes (will certainly cause trouble). Sorry.

@klimov-paul
Copy link
Member

What is all the fuzz is about?

The original PR was incomplete: it missing 'debug' folder transfer and path update at 'tests/bootsrtap.php'.
I have used my available time for opotunity to solve the issue - I do not have time for negotiations about minor adjustments.

I am here to solve the code problems and move functionality forward, so I do. The goal of the development and contributing process is to set code right for the sake of its every user - not to achieve self-affirmation or personal glory.

@zhukovra, if it is a glory you seek, try to fix some more significant issues, some of which you will be mentioned at CHANGELOG.md - then you will get it for sure.

@samdark
Copy link
Member

samdark commented Feb 1, 2018

@klimov-paul basing your own commit on existing ones would've been a bit more fair...

@zhukovra
Copy link
Author

zhukovra commented Feb 1, 2018

Further discussion is useless. Thank you @klimov-paul, I understand your position.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants