Skip to content

Conversation

@MasloMaslane
Copy link
Member

No description provided.

@MasloMaslane MasloMaslane requested a review from tonowak July 5, 2023 16:20
@MasloMaslane MasloMaslane self-assigned this Jul 5, 2023
@tonowak
Copy link
Collaborator

tonowak commented Jul 5, 2023

How does it work when there is no internet connection or it is very slow? In such cases, we shouldn't annoy the user in any way.

@MasloMaslane
Copy link
Member Author

When there’s no internet the code will work well, but I haven’t thought about slow connections

@MasloMaslane
Copy link
Member Author

MasloMaslane commented Jul 6, 2023

I added a one second timeout for the request, I think it's the best solution for slow networks

@tonowak
Copy link
Collaborator

tonowak commented Jul 6, 2023

Can this be reduced to e.g. 0.3s? We should be as non-invasive as possible. In fact, if it's possible to cache when we have last checked for updates, it would be even better, instead of executing it on each run command.

@MasloMaslane
Copy link
Member Author

Now the check is version check is running asynchronously

Copy link
Collaborator

@tonowak tonowak left a comment

Choose a reason for hiding this comment

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

Can I ask to add a few more comments? It's not immediately obvious what the new code does, what is being done concurrently, why do we have a new directory, what are possible orders of executions.

@tonowak
Copy link
Collaborator

tonowak commented Jul 6, 2023

Can we add tests? I want to be sure that the warning isn't printed when it shouldn't be printed and the code doesn't crash. So we could e.g. test the code when there is no internet connection, or with different release versions (that would require somehow changing the link or the behavior, so that testing it will be possible).

@MasloMaslane MasloMaslane requested a review from tonowak July 6, 2023 09:39
@MasloMaslane MasloMaslane merged commit 4a34229 into main Jul 6, 2023
@MasloMaslane MasloMaslane deleted the check-package-version branch July 6, 2023 10:58
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.

3 participants