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

Fix naming for Characteristic.ProgramMode #1010

Closed
wants to merge 3 commits into from

Conversation

n0rt0nthec4t
Copy link

removes extra '' on end of value Characteristic.ProgramMode.PROGRAM_SCHEDULED_MANUAL_MODE

♻️ Current situation

Seems extra '_' on end of define was unintentional

💡 Proposed solution

Changes naming Characteristic.ProgramMode.PROGRAM_SCHEDULED_MANUAL_MODE_ to Characteristic.ProgramMode.PROGRAM_SCHEDULED_MANUAL_MODE

⚙️ Release Notes

This maybe a breaking change as removes define for "PROGRAM_SCHEDULED_MANUAL_MODE_". Developers will need to updated any used of Characteristic.ProgramMode.PROGRAM_SCHEDULED_MANUAL_MODE_ to Characteristic.ProgramMode.PROGRAM_SCHEDULED_MANUAL_MODE

removes extra '_' on end of value PROGRAM_SCHEDULED_MANUAL_MODE_
@coveralls
Copy link

coveralls commented Jun 29, 2023

Pull Request Test Coverage Report for Build 5433048924

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 65.017%

Totals Coverage Status
Change from base Build 4845506880: 0.003%
Covered Lines: 7397
Relevant Lines: 10595

💛 - Coveralls

@Supereg
Copy link
Member

Supereg commented Jun 30, 2023

I'm not sure if this change is worth deliberately breaking plugins. I would either just leave it as is, or introduce the corrected PROGRAM_SCHEDULED_MANUAL_MODE while deprecating the old PROGRAM_SCHEDULED_MANUAL_MODE_ and have a grace period before completely removing it.

@n0rt0nthec4t
Copy link
Author

@Supereg fair call.. I think we should have it fixed from a naming consistency view.. I’ll update the commit with the old and new and mark the old as deprecated

Copy link
Author

@n0rt0nthec4t n0rt0nthec4t left a comment

Choose a reason for hiding this comment

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

Updated to add note PROGRAM_SCHEDULE_MANUAL_MODE_ is deprecated and will be removed

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thanks 🚀

Copy link
Member

Choose a reason for hiding this comment

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

This file is currently getting auto generated. Is there a way of editing the generator file so this doesn’t get overwritten on next generation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Well... I guess have to work out how they are auto generated.. It can take another year

Copy link
Author

Choose a reason for hiding this comment

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

Ok. So in the HAP FramWork plist, this is defined as "Program scheduled (Manual Mode)". So the really comes done to how we handle namings when enclosed in brackets/braces etc. We can correct the auto generation code to account for this?

Copy link
Member

Choose a reason for hiding this comment

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

This file is generated in generate-definitions.ts and there are some possibilities for overriding how definitions are handled in generator-configuration.ts.

Correcting the generation code sounds great. However, we probably want to manually patch the old value as a deprecated constant, such that we do not break backwards compatibility.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @Supereg I'll have a look over and see what we can do for a PR. Seems might take some more work due to structure and filechanges since those were written

Copy link
Contributor

Choose a reason for hiding this comment

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

@n0rt0nthec4t, we are close to release v1.0.0. Are you wanting this included?

Copy link
Author

Choose a reason for hiding this comment

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

I wont get todo anything anytime soon. Perhaps we'll close this PR and I'll do a new on at some point to fix the issues from the root cause

Copy link
Contributor

@donavanbecker donavanbecker Jul 9, 2024

Choose a reason for hiding this comment

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

I am going to close the pr as we will resolve this in v1.0.0 and update auto generated docs soon.

@Shaquu
Copy link
Contributor

Shaquu commented Jun 25, 2024

Probably not in scope, but I noticed some warning is those example Accessories related to special chars in names. Probably related to recent PR in beta?

@donavanbecker
Copy link
Contributor

Probably not in scope, but I noticed some warning is those example Accessories related to special chars in names. Probably related to recent PR in beta?

yes but should be resolved.

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.

5 participants