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

Deprecate validate methods with markCompleted parameter #736

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Nov 23, 2023

What has been done to verify that this works as intended?

Just reviewed the changes.

Why is this the best possible solution? Were any other approaches considered?

If a form contains errors the result should be always returned (see https://github.com/getodk/javarosa/compare/validate_improvements?expand=1#diff-42681d7b3649d584cd66ed61e3fef21179741c2414cf8371f57d49b8f212ffa0R424) and it shouldn't depend on any flag like markCompleted that indicates it should be returned only if form is already finalized or is being finalized. In Collect we had to always pass true as that parameter even if we only wanted to save current form state (even if it's just a draft) because we need the validation result what makes the flag redundant and only makes the code more difficult to understand.

Once this is merged we will need to update Collect too.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

@lognaturel
Copy link
Member

I also don't understand what the idea was with that flag. Removing it would be a breaking API change, though, so we'd want to release it as a major version. If there are other clients using this library, they're almost certainly using this method. I don't think it's worth changing.

@grzesiek2010
Copy link
Member Author

I also don't understand what the idea was with that flag. Removing it would be a breaking API change, though, so we'd want to release it as a major version. If there are other clients using this library, they're almost certainly using this method. I don't think it's worth changing.

Do we know of such clients? I thought we had forked and started maintaining javarosa for ourselves. In this case, it would be worth changing because every time we come across the code in ODK Collect where that method is used it is confusing and requires some additional investigation.

@lognaturel
Copy link
Member

lognaturel commented Nov 28, 2023

Yes, the most recent example I can think of was someone building a chat-based survey interface. People also use it in various smaller utilities.

If you feel strongly about it, how about marking the existing variant as deprecated and introducing a new signature with no args? That way it's not a breaking change but still meets your goal.

@grzesiek2010
Copy link
Member Author

Yes, the most recent example I can think of was someone building a chat-based survey interface. People also use it in various smaller utilities.

Good to know.
I still think introducing such a change would not be a disaster if someone upgrades their javarosa version, then the project would not compile, and it would be clear what should be updated. It's not like we change something and it would still allow all projects that use our library to compile and silently cause issues.
If I upgrade dependencies in Collect after every release it's common that I need to modify some parts of our code.

but if you think we definitely should avoid doing that we can do what you said above:

how about marking the existing variant as deprecated and introducing a new signature with no args? That way it's not a breaking change but still meets your goal.

@lognaturel
Copy link
Member

would not be a disaster if someone upgrades their javarosa version

JavaRosa uses semantic versioning. With a library, it's very easy to define what a major version is! If a consumer of the library needs to change their code to upgrade, then the change must be released as a major version change. I don't think this is worth a major version change but we can do the deprecation if you'd like.

If I upgrade dependencies in Collect after every release it's common that I need to modify some parts of our code.

Either libraries had a major version change, don't use semantic versioning, or made an accidental breaking change.

@grzesiek2010 grzesiek2010 force-pushed the validate_improvements branch from 20d4593 to 320edc2 Compare January 13, 2024 15:01
@grzesiek2010 grzesiek2010 changed the title Remove markCompleted flag from validate methods Deprecate validate methods with markCompleted parameter Jan 13, 2024
@lognaturel lognaturel merged commit 7eb5802 into master Jan 30, 2024
3 checks passed
@lognaturel lognaturel deleted the validate_improvements branch January 30, 2024 16:56
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.

2 participants