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

Refactor query variable names #35

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

mvorisek
Copy link
Contributor

non-functional improvement, do not assign scalar count value to the same variable which hold the query object

@mvorisek
Copy link
Contributor Author

mvorisek commented Jun 12, 2024

the PPH 5.4 CI failure is unrelated - docker-library/php#1522

@mvorisek
Copy link
Contributor Author

@AllanJard can this refactoring PR be merged?

Also one thing related to the https://editor.datatables.net/download/download?type=php release process - .gitignore and .php-cs-fixer.dist.php files should be present (files with dot in general) as user might want to run composer install and PHP CS Fixer on the release locally after some local changes.

@AllanJard AllanJard merged commit e3f951d into DataTables:master Jun 17, 2024
14 of 15 checks passed
@mvorisek mvorisek deleted the improve_var_names branch June 17, 2024 11:29
@AllanJard
Copy link
Contributor

.gitignore and .php-cs-fixer.dist.php files should be present

Actually, I've had negative feedback including the dot files before. I used to do that, but removed then when I got some "robust" feedback that they shouldn't be included.

I'm okay with them not being included at the moment - the download packages are not meant to be a development environment, the git repo exists for that. I'll keep an eye out for more feedback on this though - thank you.

@mvorisek
Copy link
Contributor Author

Do you mean .gitignore specifically? Maybe there were some other files, but the .gitignore should be present as when the release is put into VCS and composer update is run, then composer files are not ignored.

@AllanJard
Copy link
Contributor

Yeah - it was a while back, but as I recall the issue reported be a few people was that they were upset that my .gitignore was being used and thus caused some files the had expected to be committed not to be. Or something like that. I think that the downloaded PHP Editor demo package would normally be merged into the developers own project, so I'm not sure .gitignore would be relevant in such a case.

I'll add one vote to the pile for including .gitignore :)

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

Successfully merging this pull request may close these issues.

2 participants