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

SDLJingle 'constants' should be constants. #7

Closed
justinjdickow opened this Issue Oct 24, 2014 · 8 comments

Comments

Projects
3 participants
@justinjdickow
Contributor

justinjdickow commented Oct 24, 2014

currently defined as

+(NSString*) NEGATIVE_JINGLE { return @"NEGATIVE_JINGLE"; }
+(NSString*) POSITIVE_JINGLE { return @"POSITIVE_JINGLE"; }
+(NSString*) LISTEN_JINGLE { return @"LISTEN_JINGLE"; }
+(NSString*) INITIAL_JINGLE { return @"INITIAL_JINGLE"; }
+(NSString*) HELP_JINGLE { return @"HELP_JINGLE"; }

see #3 for more information

@joeljfischer

This comment has been minimized.

Show comment
Hide comment
@joeljfischer

joeljfischer Oct 29, 2014

Member

I'm assuming this was done as a way to namespace constants, but it is definitely inconsistent with best practices for the platform. All of the SDKs string constants are created using standard const variables. An example of what these should look like:

(.m file)
NSString *const SDLJingleNegative = @"NEGATIVE_JINGLE";
Member

joeljfischer commented Oct 29, 2014

I'm assuming this was done as a way to namespace constants, but it is definitely inconsistent with best practices for the platform. All of the SDKs string constants are created using standard const variables. An example of what these should look like:

(.m file)
NSString *const SDLJingleNegative = @"NEGATIVE_JINGLE";

@joeljfischer joeljfischer added this to the 4.0.0 milestone Feb 19, 2015

@joeljfischer joeljfischer removed this from the 4.0.0 milestone Mar 10, 2015

@joeljfischer joeljfischer added this to the 5.0.0 milestone Jul 31, 2015

@asm09fsu

This comment has been minimized.

Show comment
Hide comment
@asm09fsu

asm09fsu Sep 21, 2016

Contributor

This will be resolved in PR #447. Can we close this @joeljfischer ?

Contributor

asm09fsu commented Sep 21, 2016

This will be resolved in PR #447. Can we close this @joeljfischer ?

@joeljfischer

This comment has been minimized.

Show comment
Hide comment
@joeljfischer

joeljfischer Sep 22, 2016

Member

Just mention that this is fixed in #447 then

Member

joeljfischer commented Sep 22, 2016

Just mention that this is fixed in #447 then

@asm09fsu asm09fsu referenced this issue Sep 22, 2016

Merged

Stringly Typed Enums #447

4 of 4 tasks complete
@asm09fsu

This comment has been minimized.

Show comment
Hide comment
@asm09fsu

asm09fsu Sep 22, 2016

Contributor

@joeljfischer actually, after looking at this again, SDLJingle has no reason to exist anymore. This is because SDLPrerecordedSpeech contains these exact same constants. Is there another reason this exists?

Contributor

asm09fsu commented Sep 22, 2016

@joeljfischer actually, after looking at this again, SDLJingle has no reason to exist anymore. This is because SDLPrerecordedSpeech contains these exact same constants. Is there another reason this exists?

@joeljfischer

This comment has been minimized.

Show comment
Hide comment
@joeljfischer

joeljfischer Sep 22, 2016

Member

Don't think so. Needs a prop to remove probably...
On Thu, Sep 22, 2016 at 1:07 AM Alex Muller notifications@github.com
wrote:

@joeljfischer https://github.com/joeljfischer actually, after looking
at this again, SDLJingle has no reason to exist anymore. This is because
SDLPrerecordedSpeech
https://github.com/smartdevicelink/sdl_ios/blob/master/SmartDeviceLink/SDLPrerecordedSpeech.h
contains these exact same constants. Is there another reason this exists?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABr2l66KmqDOmlUUjrdAPJq4es6C3lJ4ks5qsg0DgaJpZM4CyxlO
.

Member

joeljfischer commented Sep 22, 2016

Don't think so. Needs a prop to remove probably...
On Thu, Sep 22, 2016 at 1:07 AM Alex Muller notifications@github.com
wrote:

@joeljfischer https://github.com/joeljfischer actually, after looking
at this again, SDLJingle has no reason to exist anymore. This is because
SDLPrerecordedSpeech
https://github.com/smartdevicelink/sdl_ios/blob/master/SmartDeviceLink/SDLPrerecordedSpeech.h
contains these exact same constants. Is there another reason this exists?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABr2l66KmqDOmlUUjrdAPJq4es6C3lJ4ks5qsg0DgaJpZM4CyxlO
.

@joeljfischer

This comment has been minimized.

Show comment
Hide comment
@joeljfischer

joeljfischer Sep 22, 2016

Member

Or, it's probably covered by the enum proposal.

Member

joeljfischer commented Sep 22, 2016

Or, it's probably covered by the enum proposal.

@asm09fsu

This comment has been minimized.

Show comment
Hide comment
@asm09fsu

asm09fsu Sep 22, 2016

Contributor

This will be removed in #447

Contributor

asm09fsu commented Sep 22, 2016

This will be removed in #447

@asm09fsu

This comment has been minimized.

Show comment
Hide comment
@asm09fsu

asm09fsu Jan 18, 2017

Contributor

This has been removed in develop.

Contributor

asm09fsu commented Jan 18, 2017

This has been removed in develop.

@asm09fsu asm09fsu closed this Jan 18, 2017

davidswi pushed a commit to davidswi/sdl_ios that referenced this issue May 18, 2017

Merge pull request smartdevicelink#7 in UIES/sdl-ios-lib from develop…
… to master

* commit '570db1e3378c945d0be346272a58612d15c3f860':
  removing retry logic was added by UIE. we dont need this since sdlcore start first

davidswi pushed a commit to davidswi/sdl_ios that referenced this issue Nov 15, 2017

Madhu Yaduguri
Merge pull request smartdevicelink#7 in UIES/sdl_ios from bugfix/1882…
…4 to xevo_master

* commit '574d8030c9705c9c9802059029f3832a4664ce60':
  Fix crash in SDLURLRequestTask due to self retain cycle in dispatch blocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment