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

Add '@Deprecated' annotation to update() methods #4919

Merged
merged 3 commits into from
Oct 11, 2023
Merged

Add '@Deprecated' annotation to update() methods #4919

merged 3 commits into from
Oct 11, 2023

Conversation

tati-qalified
Copy link
Contributor

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • [ X ] Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Some update() methods were missing the '@deprecated' annotation, which made the javadocs outdated. I added the annotation to these methods:

  • update(String contexts, Writer output)
  • update(Contexts contexts, Writer output)
  • update(Contexts contexts, LabelExpression labelExpression, Writer output)
  • update(Contexts contexts, LabelExpression labelExpression, Writer output, boolean checkLiquibaseTables)

Things to be aware of

This will change the javadocs documentation

Things to worry about

It's my understanding that all liquibase.update() methods are deprecated. If not, ignore this PR.

Some update() methods were missing the '@deprecated' annotation, which made the javadocs outdated.
@MalloD12
Copy link
Collaborator

MalloD12 commented Oct 2, 2023

I think it would be nice if we also add some javadoc recommending users to use CommandScope. Something like:

 /**
 * @deprecated use {@link CommandScope}
 */**

What do you guys think about it? cc: @tati-qalified @filipelautert

@tati-qalified
Copy link
Contributor Author

@MalloD12 sounds good, I see that you've already merged to master, should I push the changes here or create another PR?

@MalloD12
Copy link
Collaborator

MalloD12 commented Oct 2, 2023

@MalloD12 sounds good, I see that you've already merged to master, should I push the changes here or create another PR?

Tati, this has not been not merged yet. I just merged master into your branch. So, feel free to update this one.

Thanks,
Daniel.

@tati-qalified
Copy link
Contributor Author

Woops, I saw "merged master into master" and thought it was the other way around. I'll update the code.

@tati-qalified
Copy link
Contributor Author

Updated. Are we good to go? Any other suggestions?

@MalloD12
Copy link
Collaborator

MalloD12 commented Oct 2, 2023

Updated. Are we good to go? Any other suggestions?

Nothing else from my side, thank you @tati-qalified!

@filipelautert filipelautert merged commit f12b1b2 into liquibase:master Oct 11, 2023
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants