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

Don't use NSInvoke to get task attributes. #16

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

boutier
Copy link
Contributor

@boutier boutier commented Sep 30, 2020

I've experimented, on iOS 14.0.1, that invocation.getReturnValue(ret) raises:

[NSInvocation getArgument:atIndex:]: NULL address argument

However, using invocations seems no more needed since the properties we're
fetching are directly available from the task object.

@cla-bot
Copy link

cla-bot bot commented Sep 30, 2020

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Matthieu Boutier.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@boutier
Copy link
Contributor Author

boutier commented Sep 30, 2020

  1. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

Done.

@NathanaelA

This comment was marked as abuse.

@boutier
Copy link
Contributor Author

boutier commented Oct 3, 2020 via email

I've experimented, on iOS 14.0.1, that `invocation.getReturnValue(ret)` raises:
     [NSInvocation getArgument:atIndex:]: NULL address argument

However, using invocations seems no more needed since the properties we're
fetching are directly available from the task object.
@cla-bot
Copy link

cla-bot bot commented Oct 5, 2020

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @boutier.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

@boutier
Copy link
Contributor Author

boutier commented Oct 5, 2020

  1. Still didn't check, but the apple documentation describes NSURLSessionTask as available since iOS 7, and note that _task.description was already accessed without invocations. (https://developer.apple.com/documentation/foundation/nsurlsessiontask)

(That said, I didn't see any good reasons why these invocations are not working anymore and I'd be happy if anyone teach me.)

  1. Looking at the .prettierrc, there actually is "useTabs": trueand "singleQuote": true. Please either update these rules or do a refactoring commit since the pre-commit hook will reformat. For this time, I've removed the hook to fulfill your will since I don't know what is faulty: the prettierrc or the file format.

  2. Something bother me about signing the CLA: my postal address is likely to change and I will certainly never even thinking to change it. In addition, I don't understand the purposes of signing the CLA since it's a pull request on an Apache 2 licence project: if I change the licence, you will see it in the pull request, right? So, in fact, it's useless, or I missed something while reading what I should sign, which is worse (in this case having a "what differ from apache 2" paragraph would be nice). Please enlighten me.

@benediktveith
Copy link

Any updates?

@Exigency
Copy link

Curious to know if there is an official fix planned, would prefer to avoid going down the manual patch route.

@boutier
Copy link
Contributor Author

boutier commented Nov 2, 2020

@benediktveith, @Exigency, we're quite stuck here:

  • because the nativescript team has no lawyers, they prefer to keep a CLA that (if I well understood) has been established by the Progress team (that had lawyers),
  • because (i) I'm not a lawyer, (ii) my company don't have lawyers and (iii) I don't feel comfortable about signing something I don't really understand and for which I don't know what are the implications (e.g. shall I keep up-to-date my personal (i.e. address) informations for the rest of my life?), (iv) I deem useless to have a CLA in this case since it's not a patch in an email but a pull request coming from a repository that has an apache2 LICENCE file; then I didn't have yet signed the CLA, and… regarding of what I've written, it's quite unlikely I would did it.

So to move forwards, here are the options I see:

  1. close the pull request, report the issue and let the nativescript team redo some patch,
  2. leverage the CLA such that pull requests from an Apache2 repository are accepted,
  3. just ask to have something like "LICENCE: Apache License 2.0" in each commit message,
  4. ask to have a full copy of the Apache 2 licence in each commit message (eh!),
  5. if someone doesn't care about all of this bureaucracy, he may just clone my repo, sign the CLA and do the pull request (I guess it would be accepted?!).

@NathanWalker
Copy link
Contributor

@boutier thank you for this pull request. We're confirming a few things on older devices to know for sure and will get this merged and published by end of this week.

@NathanWalker NathanWalker merged commit 2eea608 into NativeScript:master Nov 4, 2020
@NathanWalker
Copy link
Contributor

Published in 5.0.1 - thanks again @boutier !

@boutier
Copy link
Contributor Author

boutier commented Nov 4, 2020

Thanks @NathanWalker, @NathanaelA and all the Nativescript Team for your great job! Hoping to contribute more! :-)

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.

5 participants