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

feat: Bump Eslint #781

Merged
merged 4 commits into from
Jan 5, 2025
Merged

Conversation

mykola-mokhnach
Copy link
Contributor

Eslint update also required to update the mixin logic, so the linter does not get confused too much by previous hacks.
Please check carefully if all method and property names are properly preserved because there are many and typos are possible.

dependabot bot and others added 2 commits January 3, 2025 11:43
Bumps [@appium/eslint-config-appium-ts](https://github.com/appium/appium/tree/HEAD/packages/eslint-config-appium-ts) from 0.3.3 to 1.0.1.
- [Release notes](https://github.com/appium/appium/releases)
- [Changelog](https://github.com/appium/appium/blob/master/packages/eslint-config-appium-ts/CHANGELOG.md)
- [Commits](https://github.com/appium/appium/commits/@appium/eslint-config-appium-ts@1.0.1/packages/eslint-config-appium-ts)

---
updated-dependencies:
- dependency-name: "@appium/eslint-config-appium-ts"
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
import type { LRUCache } from 'lru-cache';
import type { ExecError } from 'teen_process';
import type { StringRecord } from '@appium/types';
Copy link

Choose a reason for hiding this comment

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

Won't this require changing @appium/types from a dev dependency to a production dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to depenncies

@@ -107,7 +107,7 @@ function formatFilterSpecs (filterSpecs) {
}


class Logcat extends EventEmitter {
export class Logcat extends EventEmitter {
Copy link

Choose a reason for hiding this comment

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

Since the main index.ts file has export type * from './lib/logcat', it will now also export the Logcat type. It's probably fine but worth pointing out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

export type * from './lib/mixins';
export type * from './lib/tools';
// eslint-disable-next-line import/export
export {getAndroidBinaryPath} from './lib/tools/system-calls';
Copy link

Choose a reason for hiding this comment

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

This export looks a bit out of place in this file; maybe it could be moved back to ./lib/adb?

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 wanted to make it more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps export * from './lib/adb'; is also suboptimal. We need to be explicit about what we export

@@ -166,7 +164,7 @@ function getSdkBinaryLocationCandidates (sdkRoot, fullBinaryName) {
* of known locations or Android SDK is not installed on the
* local file system.
*/
async function getAndroidBinaryPath (binaryName) {
export async function getAndroidBinaryPath (binaryName) {
const fullBinaryName = getBinaryNameForOS(binaryName);
Copy link

Choose a reason for hiding this comment

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

Should this be _getBinaryNameForOS? Or was it originally incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one is memoized, which is what we want

@eglitise
Copy link

eglitise commented Jan 5, 2025

Checked the method/property names, all look correct 👍

Copy link

@eglitise eglitise left a comment

Choose a reason for hiding this comment

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

I wonder if there's a better way to handles the type changes that don't include moving @appium/types to a production dependency. They're not really relevant for the end user.

@mykola-mokhnach mykola-mokhnach merged commit 9613100 into appium:master Jan 5, 2025
10 checks passed
@mykola-mokhnach mykola-mokhnach deleted the eslint_upd branch January 5, 2025 19:44
github-actions bot pushed a commit that referenced this pull request Jan 5, 2025
## [12.8.0](v12.7.4...v12.8.0) (2025-01-05)

### Features

* Bump Eslint ([#781](#781)) ([9613100](9613100))
Copy link

github-actions bot commented Jan 5, 2025

🎉 This PR is included in version 12.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants