Skip to content

Constanst.setRefValue() was always null #196

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 25, 2022
Merged

Constanst.setRefValue() was always null #196

merged 2 commits into from
Apr 25, 2022

Conversation

fjtirado
Copy link
Collaborator

Setting the ref value when constant is an uri to allow implementations
to try load them in case default loading fails

@fjtirado fjtirado requested a review from tsurdilo as a code owner April 22, 2022 15:31
@fjtirado fjtirado force-pushed the constant_ref_value branch from f7bebd5 to b3aeb9f Compare April 22, 2022 15:35
ricardozanini
ricardozanini approved these changes Apr 22, 2022
View reviewed changes
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

@tsurdilo I think we can set up a formatting profile for the SDK (see the imports). I'll do it. So, with a mvn clean install the files are automatically formatted.

@fjtirado fjtirado force-pushed the constant_ref_value branch 2 times, most recently from 5073fa4 to ef8e833 Compare April 22, 2022 15:46
@fjtirado
Copy link
Collaborator Author

fjtirado commented Apr 22, 2022 •
edited
Loading

@tsurdilo I think we can set up a formatting profile for the SDK (see the imports). I'll do it. So, with a mvn clean install the files are automatically formatted.

I removed the changed imports, but yes, template is a nice idea, thanks @ricardozanini

@fjtirado fjtirado force-pushed the constant_ref_value branch from ef8e833 to 7611b39 Compare April 22, 2022 16:13
@tsurdilo
Copy link
Contributor

Thanks @fjtirado , could we please add a test for this change as well?

@tsurdilo
Copy link
Contributor

@manick02 can you please work with @fjtirado on this pr? Let me know when its good to merge. Would be nice to:

  1. add a test for it
  2. maybe we should have a null check for constantsFileDef
  3. whatever else you see might be needed
    Thanks!

@manick02
Copy link
Contributor

@manick02 can you please work with @fjtirado on this pr? Let me know when its good to merge. Would be nice to:

  1. add a test for it
  2. maybe we should have a null check for constantsFileDef
  3. whatever else you see might be needed
    Thanks!

sure @tsurdilo

@manick02
Copy link
Contributor

@tsurdilo I think we can set up a formatting profile for the SDK (see the imports). I'll do it. So, with a mvn clean install the files are automatically formatted.

We have this plugin already in java sdk @ricardozanini

com.coveo
fmt-maven-plugin
${version.fmt-maven-plugin}

@fjtirado
Constanst.setRefValue() was always null
9c68ef2 
Setting the ref value when constant is an uri to allow implementations
to try load them in case default loading fails

Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
@fjtirado fjtirado force-pushed the constant_ref_value branch from 7611b39 to 9c68ef2 Compare April 25, 2022 10:14
@fjtirado
Copy link
Collaborator Author

@tsurdilo
Test added

@fjtirado fjtirado requested a review from manick02 April 25, 2022 11:25
@fjtirado
Changes after comments
a924fe0 
Signed-off-by: Francisco Javier Tirado Sarti <[email protected]>
@fjtirado fjtirado force-pushed the constant_ref_value branch from 1fafaf3 to a924fe0 Compare April 25, 2022 11:32
@manick02
Copy link
Contributor

@tsurdilo Looks Good from my side

@tsurdilo
Copy link
Contributor

tsurdilo commented Apr 25, 2022 •
edited
Loading

@manick02 thanks!!
thanks for the contribution @fjtirado

@tsurdilo tsurdilo merged commit 2397b88 into serverlessworkflow:main Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cb-manick cb-manick cb-manick left review comments

@ricardozanini ricardozanini ricardozanini approved these changes

@tsurdilo tsurdilo Awaiting requested review from tsurdilo

@manick02 manick02 Awaiting requested review from manick02

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fjtirado @tsurdilo @manick02 @ricardozanini @cb-manick