Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix fputcsv annotation #318

Merged
merged 1 commit into from
Jan 11, 2022
Merged

Conversation

Jean85
Copy link
Contributor

@Jean85 Jean85 commented Jan 5, 2022

During #316 I restricted the type of the $field argument, and that triggered my static analysis tools while importing the fix.

As shown in https://3v4l.org/ahdmn, the original fputcsv function will try to cast everything to string, so every scalar is safe, array are risky (they become Array), objects explode, with the exception of stringable ones.

@Kharhamel
Copy link
Collaborator

This looks good. The CI error seems to come from a cache error (I had the same on another PR) so I think we can safely merge it. How sure are you of your fix?

@Jean85
Copy link
Contributor Author

Jean85 commented Jan 5, 2022

Umh, the PR seems kinda messed up, because the fix was already merged in #316...
Maybe you erased that with your forced push?

@Kharhamel
Copy link
Collaborator

What you mean? Which force push? I only ever force-push to my own branches, never to master.

I also fixed the cache, which should solve your CI error. You just need to pull master to benefit from the change.
Also could you squash all of your commits into a single one? That would look cleaner

@Jean85
Copy link
Contributor Author

Jean85 commented Jan 6, 2022

GitHub doesn't agree... That's what I see:
immagine

You force-pushed, erasing 6b3abbe which was @dbrekelmans merging my #316 into master.

That's why this PR is messy: it's #316 + https://github.com/thecodingmachine/safe/pull/318/files/9f63c60d5f1e746873629fd5447e6b80f7622770..246d93cd3a7d52d7beb33028918d64de45a24329

@Kharhamel
Copy link
Collaborator

Kharhamel commented Jan 6, 2022

That's weird. How did I do that. If it is really messed up, the easy way to fix is to simply destroy the commits (but not the changes), stash the changes, start over from a clean master branch and commit. So something like this:

git reset _hashOfTheCommitBeforeYourChanges_
git stash
git checkout master
git pull master
git checkout -b _newBranch_
git stash pop
git commit

If you need help, we can try to meet on discord and fix it together

@Jean85
Copy link
Contributor Author

Jean85 commented Jan 10, 2022

Done!

@codecov-commenter
Copy link

Codecov Report

Merging #318 (74d8991) into master (418777f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #318   +/-   ##
=========================================
  Coverage     48.48%   48.48%           
  Complexity      308      308           
=========================================
  Files            15       15           
  Lines           794      794           
=========================================
  Hits            385      385           
  Misses          409      409           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 418777f...74d8991. Read the comment docs.

@Kharhamel
Copy link
Collaborator

Damn i just created a new release. I'll do another one i guess

@Kharhamel Kharhamel merged commit aeca670 into thecodingmachine:master Jan 11, 2022
@Jean85 Jean85 deleted the fix-fputcsv branch January 12, 2022 07:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants