Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

kobotoolbox: switch baseURL to baseUrl #989

Merged
merged 4 commits into from
Feb 10, 2025

Conversation

hunterachieng
Copy link
Contributor

@hunterachieng hunterachieng commented Feb 10, 2025

Summary

Switch from using baseURL to baseUrl in kobotoolbox

Fixes #985

Details

All adaptors use baseUrl as opposed to kobotoolbox that uses baseURL. We have switched to baseUrl but still allow users to send requests with baseURL but log a warning.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to
know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our
Responsible AI Policy

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • If this is a new adaptor, added the adaptor on marketing website ?
  • If this PR includes breaking changes, do we need to update any jobs in
    production? Is it safe to release?
  • Are there any unit tests?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.
  • Have you ticked a box under AI Usage?

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>

let baseURL = baseUrl || state.configuration.baseURL;

if (!baseUrl) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work because if baseURL is set, the baseUrl var will have a value and this block won't be entered. Did you test it manually?

I think you should be more explicit and have a whole if statement around state.configuration.baseURL, and do the warning and assignment inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did test it, but let me change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>

let baseURL;

if (state.configuration.baseURL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok but this way, if baseURL and baseUrl are set, baseURL will win AND users will get an incorrect warning.

I find it kinda weird too that the code uses the wrong styling of baseURL. It would feel cleaner if code and config were consistent and baseURL was the outlier.

So:
baseUrl needs to be the main variable
`if (!baseURL && state.configuration.baseURL) { /* warn and assign */ }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't rush your solution! It is very expensive to keep revisiting little fixes like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated it with baseUrl as the main variable

Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

Thank you!

@josephjclark josephjclark merged commit 9628548 into epic/kobo Feb 10, 2025
2 checks passed
@josephjclark josephjclark deleted the feature/985-kobo-baseURL branch February 10, 2025 12:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants