Skip to content

Removing manufacture_date from battery table #8521

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Removing manufacture_date from battery table #8521

wants to merge 1 commit into from
+0 −3

Conversation

JodhwaniMadhur
Copy link
Contributor

  • Read the CONTRIBUTING.md guide at the root of the repo.
  • Ensure the code is formatted building the format_check target.
    If it is not, then move the committed files to the git staging area,
    build the format target to format them, and then re-commit.
    More information is available on the wiki.
  • Ensure your PR contains a single logical change.
  • Ensure your PR contains tests for the changes you're submitting.
  • Describe your changes with as much detail as you can.
  • Link any issues this PR is related to.
  • Remove the text above.

Resolves #8266

Removed the manufacture_date from battery table for macOS.

@JodhwaniMadhur JodhwaniMadhur requested review from a team as code owners January 1, 2025 18:05
directionless
directionless previously approved these changes Jan 6, 2025
View reviewed changes
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Code seems okay, and I trust the diagnosis in #8266

I'll note this is a breaking change, but I'm not sure the column is worth adding a hidden one for to preserve any explicit queries. (I think cleaner code is better here)

@JodhwaniMadhur
Copy link
Contributor Author

@zwass @Smjert @mike-myers-tob could one of you please merge this?

@michael-myers
Copy link
Contributor

@JodhwaniMadhur I know the PR was approved already but I have two comments

  1. If we are removing the column, why not delete lines 138 through 159, rather than just line 158? That whole 'if' clause is just the API call that isn't working.

  2. if you see my comments on issue 8521 I think we can salvage this functionality without deleting the column, but it requires if others can confirm my observations there.

@directionless
Copy link
Member

I'm holding off on merging this for a couple reasons --

  1. It sounds like we could get this data, so investigation there seems worthwhile
  2. Mike's comments above

@directionless directionless dismissed their stale review February 5, 2025 11:37

sounds like we could get it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@directionless directionless directionless left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
API change macOS virtual tables
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: Remove manufacture_date from battery table
3 participants
@JodhwaniMadhur @michael-myers @directionless