-
Notifications
You must be signed in to change notification settings - Fork 2
Initial project structure. #1
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
Conversation
heiglandreas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Crell for starting!
jrfnl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glanced through it, haven't checked everything, but I kind of think this needs some more work....
phpstan.neon
Outdated
| paths: | ||
| - src | ||
| - tests | ||
| ignoreErrors: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, as long as there is no code, there should not be any ignores...
Only add those ignores which are needed, when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it very helpful to have certain errors turned off in advance, when I know I'm going to turn them off. It saves me the hassle of having to remember how to do it if I do eventually add code that trips over that rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment about storing these in a separate branch to be pulled later when it becomes relevant.
|
Oh, one more remark which I forgot to add - I'd strongly recommend for the And for the These projects will automagically pick up on either file and the non-dist file overrules the |
|
Does anyone actually do that? I never have. |
|
Yes, I do. Everywhere and all the time. |
|
In case anyone is wondering, here are some reasons of why I do this:
Just to give you some idea. |
|
@Crell Please "unresolve" threads which have gotten replies. Those threads are not resolved (and some others aren't either, but are waiting replies from others). Closing discussions this way dismisses valid concerns. |
I regularily do that. Regardles of that it is IMO good citizenship.to do it that way. It doesn't hurt the project and it opens possibilities for contributors. |
Co-authored-by: Jaap van Otterdijk <jaap@ijaap.nl>
jrfnl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet more feedback.
| .env | ||
| vendor | ||
| build | ||
| composer.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking - we don't actually need this if we make a small change in the composer.json file (though it doesn't do any harm to leave this line in the .gitignore anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still keep it. And if only to not confuse people to "Why is there no composer.lock file" ... They might have overlooked the lock: false in the composer.json 🙈
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
jaapio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good for a first version, let's merge this and start improvements via new pr's
heiglandreas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nitpicks that are not at all relevant and can - if we decide upon it - be modified later on.
So from my side this is a Go 🚀
I'll merge so that we can continue with the first actual attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH: I'd rather see us using phpcs instead. Less opinionated and more flexible. But that is something that we can change along the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't comment on it as @Crell specifically mentioned above they wanted to use PERCS and PHPCS doesn't yet have a good ruleset for that, but it looks like something is happening there, so 🤞🏻 we may have a PERCS ruleset to use soonish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong preference between the two. I am more familiar with php-cs-fixer, so that's what I used. But "can let us apply PER-CS 2" is the main criteria, so whatever gives solid PER-CS support works for me.
| .env | ||
| vendor | ||
| build | ||
| composer.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still keep it. And if only to not confuse people to "Why is there no composer.lock file" ... They might have overlooked the lock: false in the composer.json 🙈
|
|
||
| - **One pull request per feature** - If you want to do more than one thing, send multiple pull requests. | ||
|
|
||
| - **Send coherent history** - Make sure each individual commit in your pull request is meaningful. If you had to make multiple intermediate commits while developing, please [squash them](http://www.git-scm.com/book/en/v2/Git-Tools-Rewriting-History#Changing-Multiple-Commit-Messages) before submitting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **Send coherent history** - Make sure each individual commit in your pull request is meaningful. If you had to make multiple intermediate commits while developing, please [squash them](http://www.git-scm.com/book/en/v2/Git-Tools-Rewriting-History#Changing-Multiple-Commit-Messages) before submitting. | |
| - **Send coherent history** - Make sure each individual commit in your pull request is meaningful and atomic. Make sure each commit-message not only explains the **what** but also the **why**! |
Commits should be atomic. If there are multiple commits necessary they should be preserved to understand the process. The "Multiple intermediate commits" sounds like there should only be one commit per PR which I would rather not have people to get the impression of. They should use atomic commits and preserve them.
And if we want meaningful commit-messages it is unfair towards the contributor to then squash them into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inherited all the way from the PHP League template, I think. I've barely looked at it in years. Follow ups to change this part are fine by me, I don't have strong feelings about it, other than we give people good guidance.
|
|
||
| ## Working Group | ||
|
|
||
| <!-- Add your own name here --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <!-- Add your own name here --> | |
| <!-- Add your own name here --> | |
| - [Andreas Heigl](link-heiglandreas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include a sentence that this is a volunteer project without funding and that we are not able to pay bug bounties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can. As I said, I stole this doc straight from my own template where I don't say that, but maybe should. 😄 Easy follow up for someone.
A basic project setup, derived mostly from my personal project setup. Please use "suggests" to add your own name and link to the Working Group section. We can tweak this over time, but I wanted to get something in here so we could start adding code.
I am including PHPStan and PHP-CS-Fixer at strict settings. I went with PHPStan because I know it better than Psalm, and it's more widely used. I went with CS-Fixer because its support for PER-CS is more up to date.
I targeted PHP 8.2 as a baseline somewhat arbitrarily. I think it's reasonable for different attributes to have different required versions. I'm fine with increasing the min version, but I wouldn't go lower than 8.2 as 8.1 goes out of security support at the end of this year anyway.