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

Linted most of the linty bits of app manager #35653

Merged
merged 3 commits into from
Jan 24, 2025
Merged

Conversation

orangejenny
Copy link
Contributor

Technical Summary

Prep for migrating app manager to webpack.

All that's left is app_manager/js/forms/case_config_ui, app_manager/js/settings/commcare_settings, and all the use strict warnings. I don't have the wherewithal to do those right now.

Safety Assurance

Safety story

Code review is a sufficient safety check. The one == to === is the only not-quite-mechanical change, and I'm pretty confident that $jqueryThing.val() always return a string, number, or array (docs).

Automated test coverage

Not really.

QA Plan

No.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the product/invisible Change has no end-user visible impact label Jan 22, 2025
@orangejenny orangejenny mentioned this pull request Jan 22, 2025
3 tasks

// Callback for when comment update is successful
// Arguments: name, comment
var _updateCallbackFn = function () {};
Copy link
Contributor

Choose a reason for hiding this comment

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

is the linter complaining about the unused name and comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@MartinRiese
Copy link
Contributor

Seems pretty save but maybe merge after deploy so it can sit on staging for longer than a few hours

@orangejenny
Copy link
Contributor Author

@MartinRiese sounds reasonable, I'll wait to merge

@orangejenny
Copy link
Contributor Author

Resolved conflict, ready to merge now.

@orangejenny orangejenny merged commit 3caa6f1 into master Jan 24, 2025
13 checks passed
@orangejenny orangejenny deleted the jls/lint-app-manager branch January 24, 2025 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants