Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Justin Mclean
Administrator
Hi,

I notice in this check in you changed all of the event contestants to hard coded strings i.e. HTTPConstants.STATUS - > “httpStatus”.

Any reason for that change especially given the conversion on the dev list recently that it doesn’t matter and the using constants is the preferred way?

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Harbs
Not really. I wrote the original code and I was making these changes anyway and some of the lines were really long.

> On Jul 15, 2017, at 4:34 AM, Justin Mclean <[hidden email]> wrote:
>
> Hi,
>
> I notice in this check in you changed all of the event contestants to hard coded strings i.e. HTTPConstants.STATUS - > “httpStatus”.
>
> Any reason for that change especially given the conversion on the dev list recently that it doesn’t matter and the using constants is the preferred way?
>
> Thanks,
> Justin

Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Justin Mclean
Administrator
Hi,

Can you please revert those changes please - they are not required and not in agreement with what we have discussed on the list.

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Justin Mclean
Administrator
Hi,

I can see these event name changes haven’t been reverted yet. Do you want me to do that for you?

I also note the code is using "”+requestStatus to convert a number to a string. Any reason for not using the toString or String(requestStatus) instead?

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Harbs

> On Jul 18, 2017, at 5:41 AM, Justin Mclean <[hidden email]> wrote:
>
> Hi,
>
> I can see these event name changes haven’t been reverted yet. Do you want me to do that for you?

No. The consensus was that the compiler needs improvement to replace static constants with string literals and for now to not enforce one way or the other.

Until the compiler is fixed, my personal preference remains to use string literals.

>
> I also note the code is using "”+requestStatus to convert a number to a string. Any reason for not using the toString or String(requestStatus) instead?

It’s more concise.

> Thanks,
> Justin

Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

piotrz
Harbs,

If there is no jira around there about that - please raise one once you get a chance.

Thanks,
Piotr
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Harbs
It’s kind of low on my priority list right now… ;-)

> On Jul 18, 2017, at 8:53 AM, piotrz <[hidden email]> wrote:
>
> Harbs,
>
> If there is no jira around there about that - please raise one once you get
> a chance.
>
> Thanks,
> Piotr
>
>
>
> -----
> Apache Flex PMC
> [hidden email]
> --
> View this message in context: http://apache-flex-development.2333347.n4.nabble.com/Re-git-commit-flex-asjs-refs-heads-develop-Added-support-for-JS-upload-progress-events-tp63275p63377.html
> Sent from the Apache Flex Development mailing list archive at Nabble.com.

Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

piotrz
No problem! We have your words on Dev list! :P :)

Piotr
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Justin Mclean
Administrator
In reply to this post by Harbs
Hi,

> No. The consensus was that the compiler needs improvement to replace static constants with string literals and for now to not enforce one way or the other.

I’m not sure how you get that from the two threads he had on this. I’m happy to go through and summaries the thread for you if you want or if you prefer let call a VOTE and abide by the results of that?

> Until the compiler is fixed, my personal preference remains to use string literals.

You personal preference that may be that but you changed code so that it not longer uses constants.

>> I also note the code is using "”+requestStatus to convert a number to a string. Any reason for not using the toString or String(requestStatus) instead?
>
> It’s more concise.

It’s also buggy (for large numbers for instance) so I would take care in using it, if you were worried about null or undefined then String(requestStatus) will do what you need and is much clearer to understand.

Thanks,
Justin
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Alex Harui-2
Justin, this is not worth the time being spent on it.

Can we focus more on the priorities from the summit [1]?  Or maybe a
feature like AMF that some folks really need?

[1]
https://lists.apache.org/thread.html/3ef2cc9fcd17bbf0c81b38c35586790255277f
bb4f727db8882920ec@%3Cdev.flex.apache.org%3E

-Alex

On 7/17/17, 11:28 PM, "Justin Mclean" <[hidden email]> wrote:

>Hi,
>
>> No. The consensus was that the compiler needs improvement to replace
>>static constants with string literals and for now to not enforce one way
>>or the other.
>
>I’m not sure how you get that from the two threads he had on this. I’m
>happy to go through and summaries the thread for you if you want or if
>you prefer let call a VOTE and abide by the results of that?
>
>> Until the compiler is fixed, my personal preference remains to use
>>string literals.
>
>You personal preference that may be that but you changed code so that it
>not longer uses constants.
>
>>> I also note the code is using "”+requestStatus to convert a number to
>>>a string. Any reason for not using the toString or
>>>String(requestStatus) instead?
>>
>> It’s more concise.
>
>It’s also buggy (for large numbers for instance) so I would take care in
>using it, if you were worried about null or undefined then
>String(requestStatus) will do what you need and is much clearer to
>understand.
>
>Thanks,
>Justin

Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

piotrz
Alex,
 +1
Thanks for such reminder! Really wish someone could start look into that!

Piotr
Reply | Threaded
Open this post in threaded view
|

Re: git commit: [flex-asjs] [refs/heads/develop] - Added support for JS upload progress events

Justin Mclean
Administrator
In reply to this post by Alex Harui-2
Hi,

> Can we focus more on the priorities from the summit [1]?  

Sonar Cube analysis was discussed at the summit i.e. items 5 and 6 which it can help with.

> Or maybe a feature like AMF that some folks really need?

I’m not interested in AMF support so it’s unlikely I’ll work on it.

Thanks,
Justin