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: flet doctor CLI command #4803

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Gordon-Burns
Copy link

@Gordon-Burns Gordon-Burns commented Jan 30, 2025

Description

Test Code

# Test code for the review of this PR

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • I signed the CLA.
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing tests pass locally with my changes
  • I have made corresponding changes to the documentation (if applicable)

Screenshots

Additional details

Summary by Sourcery

New Features:

  • Introduce the flet doctor command to diagnose potential issues with the Flet environment.

@CLAassistant
Copy link

CLAassistant commented Jan 30, 2025

CLA assistant check
All committers have signed the CLA.

@Gordon-Burns
Copy link
Author

Tests passed and screenshot of output attached - Ready for review
image

@ndonkoHenri ndonkoHenri mentioned this pull request Jan 30, 2025
5 tasks
@ndonkoHenri
Copy link
Contributor

Thanks for your contribution! It's awesome.

Will have a look at it today and return to you asap.

@ndonkoHenri
Copy link
Contributor

  • Running flet doctor -v shows the following (we have twice thesame info):
    image

  • Also, seems like the output is printed all at once, which takes a while. I will suggest we print info step by step like in the flet build command (where we make use of rich's status) or flutter doctor:
    flutter-doctor-in-action

@Gordon-Burns
Copy link
Author

Thanks @ndonkoHenri I have solved the duplicate and print issues as well as adding some additional checks in the verbose option
image

@ndonkoHenri
Copy link
Contributor

ndonkoHenri commented Feb 1, 2025

Great! Thanks for the changes @Gordon-Burns.

Some other remarks:

  • Can you please remove all the lines printing strings containing "==" ? And also "Detailed check completed" at the end. Let's please solely have the important output of the command being printed out.
  • In your methods (ex: check_flet_version, check_os_info), no need to return a value as it won't be used. Then in the handle method we can just call the different methods, without having to store them in variables.

@Gordon-Burns
Copy link
Author

Gordon-Burns commented Feb 3, 2025

@ndonkoHenri Picked this up over the weekend and have addressed feedback by removing the unnecessary characters. Also changed the check_vitrual_env as it did not account for someone using conda
image

@Gordon-Burns
Copy link
Author

@ndonkoHenri Just wanted to check in if there are anymore changes required or if this request is good ?

@FeodorFitsner
Copy link
Contributor

Re: "Flutter is not installed" on the last screenshot - in Flet 0.26.0 flet build is installing Flutter SDK, JDK and Android SDK if they are not installed. Other words, we are not asking/encouraging users to install those anymore. I don't know if it makes sense for flet doctor to check/warn on that. What do you think?

@Gordon-Burns
Copy link
Author

Hi @FeodorFitsner the issue asked for this to be included in the checks- but if you feel it's no longer needed I am happy to remove?

@FeodorFitsner
Copy link
Contributor

Thanks. I think it could be safely removed. Flet does no longer rely on Flutter installed by the user, but uses its private install.

@Gordon-Burns
Copy link
Author

Sure, I will pick this up over the weekend and push changes

@Gordon-Burns
Copy link
Author

@FeodorFitsner , Thanks again for the feedback this has now been addressed and pushed

@ndonkoHenri
Copy link
Contributor

Sorry @Gordon-Burns, got distracted/occupied by some other stuffs lately.

I made some last modifications as you might have noticed: the admin privileges are non-important, and it's check has been removed.

Your PR looks good to be merged - thanks again for picking this up!

@Gordon-Burns
Copy link
Author

Thanks @ndonkoHenri no worries on removing the other function

@ndonkoHenri ndonkoHenri changed the title Add doctor command feat: flet doctor CLI command Feb 10, 2025
@ndonkoHenri ndonkoHenri linked an issue Feb 10, 2025 that may be closed by this pull request
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flet doctor CLI command
4 participants