Conversation
|
@VaryaGet take a look. |
|
@MaxS-9779 Build is failing, fix it. |
ArtemGet
left a comment
ArtemGet
left a comment
There was a problem hiding this comment.
@MaxS-9779 Good, tnx for contributing! I suggest fixing build and comments below. I believe it would be better to create separate pr containing EYml and EYmlStr before doing this.
src/test/java/io/github/artemget/entrys/profile/EValProfTest.java
Outdated
Show resolved
Hide resolved
| public EValProf(final String key, final Entry<String> content, final String path) { | ||
| super( | ||
| () -> { | ||
| YamlMapping root; |
There was a problem hiding this comment.
I beleive if we have same logic to get value from yaml in two places - there is a reason to create separate class EYml.
| ); | ||
| } | ||
|
|
||
| private static YamlNode getProfileNode(final YamlMapping root) { |
There was a problem hiding this comment.
As I mentioned above, it is better to create separate classes for this - EYml and EYmlStr
Add more tests; Devide ex handling; Avoid type casting
| result = new EVal(key, parsePath(path, profile)).value(); | ||
| } | ||
| } catch (final EntryException exception) { | ||
| result = new EVal(key, content).value(); |
There was a problem hiding this comment.
Imagine 2 cases of application.yaml:
profile: {PROFILE} //imagine user did not set any env value
age:123
and
age:123 //in this application.yaml user did not specify profile at all.
In first case we MUST rethrow throw an exception to show user that there is a mistake - env is not set (but it needs to be set in case user specified profile by himself)
In second case we are ok with just returning value from application.yaml (as you did)
So, I would do next:
1)Check either there is a profile mention in config (get it via Yaml parsing similar to how its done in EVal, I suggest to move this logic to EYml, EYmlStr classes first in separate pr)
2)If there is profile mention - get profile name (you must handle exception and rethrow it with custom message)
2.1)get value via EVal with new config path - application-{profile_name}.yaml
3)If there is no profile mention - get value from config path that user passed in constructor.
| final String profile; | ||
| try { | ||
| profile = new EVal("entrys.profile", path).value(); | ||
| if (profile.isBlank()) { |
There was a problem hiding this comment.
Im not sure that this case would be possible.
There was a problem hiding this comment.
@MaxS-9779 I strongly recommend:
0)Fix build/checkstyle, all jobs must be green otherwise pull-request will be ignored. If there is any reasonable cause for jobs to be failed - write it down.
1)Move EYaml to separate pull-request, add tests. Pay attention that EYaml must be similar to EJson not EVal.
--- After 1 is done and merged ---
2)Rethink constructor parameters in EValProf to make it testable and reusable, add tests.
3)Rethink exception messages, we must provide user a valuable messages with proper context. There is a huge difference between missing profile value and missing profile mapping at all. There must be tests to check which message user will get in different cases.
| * | ||
| * @since 0.4.0 | ||
| */ | ||
| public class EYaml extends ESafe<String> { |
There was a problem hiding this comment.
Lets move it to separate pr and add tests.
|
|
||
| /** | ||
| * Configuration properties entry. | ||
| * By default gets entries from "src/main/resources/application.yaml" |
There was a problem hiding this comment.
I dont think that its a good idea, because we already have EVal etc.
The idea of this class might be the same with the EJson. If you want to get values via expression (as done in EVal and your class) it is possible.
| return root; | ||
| } | ||
|
|
||
| private static String ejected(final YamlNode node) throws EntryException { |
There was a problem hiding this comment.
Seems like copy-paste from EVal. But I think you would not need this code anymore (read comments above).
| super( | ||
| () -> { | ||
| final String result; | ||
| if (new EContains(new EYaml("entrys.profile", path)).value()) { |
There was a problem hiding this comment.
How would you know either entry.profile mapping not exists or just its value is missed?
| * @since 0.4.0 | ||
| */ | ||
| @SuppressWarnings({"PMD.AvoidDuplicateLiterals", "PMD.TooManyMethods"}) | ||
| final class EValProfTest { |
There was a problem hiding this comment.
I dont see any positive tests with actually using profile value (because you made constructor that is impossible to unit-test).
| @Test | ||
| void parsesStringWrap() throws EntryException { | ||
| Assertions.assertEquals( | ||
| " First line.\n Second line.\n", |
2 participants
No description provided.