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

Fix #923 Enabled support for DefaultValuePropertyDriver #924

Merged

Conversation

dbojdo
Copy link
Contributor

@dbojdo dbojdo commented Mar 21, 2023

Q A
Bug fix? no
New feature? yes
Doc updated yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #923
License MIT

Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

Thanks @dbojdo for contribution! Changes looks fine however I wonder if it will introduce some breaking changes to the flow? May it should be configurable?

@dbojdo
Copy link
Contributor Author

dbojdo commented Mar 21, 2023

Well, we may go the same way as for the enums. Introduce a new config property like: default_value_support: true/false (false by default) and then respect it in the extension.

@scyzoryck
Copy link
Collaborator

I think it will be better way to safely release it as non major release :)

@dbojdo
Copy link
Contributor Author

dbojdo commented Mar 22, 2023

I think it will be better way to safely release it as non major release :)

Good for me!

@goetas
Copy link
Collaborator

goetas commented Jun 9, 2023

@dbojdo let me know then the config option is implemented, we can merge it

@Kozzi11
Copy link

Kozzi11 commented Jul 20, 2023

@dbojdo what is the status of this? Are you willing to continue on this PR (add support for config) or can somebody else continue on it?

@dbojdo
Copy link
Contributor Author

dbojdo commented Jul 20, 2023

Hey! Don't mind someone else to take over as I'm super short on time now. There's not much to be done but can't afford the context switch now.

@Kozzi11
Copy link

Kozzi11 commented Jul 21, 2023

Perfect, I have cretead PR (dbojdo#1) to your branch with few modification can you merge it so changes will be reflected here? Thank you @dbojdo

@dbojdo
Copy link
Contributor Author

dbojdo commented Jul 21, 2023

@Kozzi11 this that's been merged, thanks for your help!

Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

Thanks for contribution!
@goetas - Could you run the tests & merge if they passes, please? :) Unfortunately I do not have access to merge in this repository :(

@Kozzi11
Copy link

Kozzi11 commented Aug 22, 2023

@goetas ping

@goetas goetas closed this Aug 28, 2023
@goetas goetas reopened this Aug 28, 2023
@goetas goetas merged commit 3671b41 into schmittjoh:master Aug 28, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants