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

Disable property type cast to fix issue gh-291 #781

Merged

Conversation

Swordsman-Inaction
Copy link

Fixes #291

Two changes(Breaking changes):

  • Disable property type cast, now property value can only be String
  • Add extension functions for int and float properties.

Before:

  • getKoin().getProperty<Int>("int.value")
  • getKoin().getProperty<Float>("float.value")

Now:

  • getKoin().getIntProperty("int.value")
  • getKoin().getFloatProperty("float.value")

@arnaudgiuliani arnaudgiuliani added this to the 2.1.6 milestone Apr 22, 2020
@arnaudgiuliani arnaudgiuliani added core status:accepted accepted to be developed labels Apr 22, 2020
@arnaudgiuliani arnaudgiuliani added status:checking currently in analysis - discussion or need more detailed specs contribution and removed status:accepted accepted to be developed labels May 15, 2020
Copy link
Member

@arnaudgiuliani arnaudgiuliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, thanks for your contribution.

Just renamed it as KoinPropertyExt.kt, can you test it?

@arnaudgiuliani arnaudgiuliani changed the base branch from master to 2.1.x May 15, 2020 07:56
@arnaudgiuliani
Copy link
Member

Please also check conflicts

@arnaudgiuliani arnaudgiuliani added status:waiting_feedback status:accepted accepted to be developed and removed status:checking currently in analysis - discussion or need more detailed specs labels May 15, 2020
@Swordsman-Inaction
Copy link
Author

@arnaudgiuliani Updated and tested.
But the MVVMActivity in androidx-samples project is crashing because of this change: 53c07fb#diff-2263db8ed8a57c624f749b4114746255R45

@jeziellago
Copy link

@arnaudgiuliani Updated and tested.
But the MVVMActivity in androidx-samples project is crashing because of this change: 53c07fb#diff-2263db8ed8a57c624f749b4114746255R45

#795 should fix androidx-samples.
https://github.com/InsertKoinIO/koin/pull/795/files#diff-2263db8ed8a57c624f749b4114746255R53

@arnaudgiuliani
Copy link
Member

Ok, will check that asap 👍

@arnaudgiuliani
Copy link
Member

just merged the PR

@arnaudgiuliani
Copy link
Member

can you double check @Swordsman-Inaction ? 🙏

@Swordsman-Inaction
Copy link
Author

@arnaudgiuliani Just merged 2.1.x, all good now.

@arnaudgiuliani arnaudgiuliani merged commit 39f1c3e into InsertKoinIO:2.1.x May 27, 2020
@chrisjenx
Copy link

Don't wanna be that guy - but yo - this really does not go into a bugfix release! this is a Major a release at best, Minor release at worst. If you are going to use semver at least respect it!

@Satook
Copy link

Satook commented Sep 9, 2020

I agree with @chrisjenx. This is a backward incompatible change. I'm stuck on 2.1.5 until I change how I pass config data around my app.

FYI: I'm using Ktor which has a richer ApplicationConfig (it supports nesting and lists of values). Means I'll have to stop using properties and move it over to an injectable parameter.

Would a patch that keeps the same external interface (getProperty, getIntProperty, getFloatProperty) be accepted? I'd add getAnyProperty or getTypedProperty and swap the map back to <string, any>. This way we can keep the string oriented interfaces for people who just want a flat PropertyList style but give others in my situation a good migration path.

@@ -0,0 +1,63 @@
package org.koin.ext
Copy link
Contributor

@antohaby antohaby Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file isn't complete enough.
On the previous version getProperty was accessible from Scope as well.

For example:

single {
    ConnectionPool (
        maxConnections = getIntProperty("CONNECTION_POOL__MAX_CONNECTIONS")
    )
}

is not going to work, but should IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core status:accepted accepted to be developed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Number-like strings in koin.properties
6 participants