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

Create Requirements webview component #1498

Merged
merged 12 commits into from
May 3, 2024
Merged

Conversation

sagerb
Copy link
Collaborator

@sagerb sagerb commented May 1, 2024

Intent

Resolves #1343
Resolves #1345

This PR implements a Python package section within the webViewView as a contextual section to the EasyDeploy.

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Approach

Appropriately replicated functionality from old requirements.ts view, but now operating within the confines of our WebViewView Vue app.

All expected behavior should work:

  1. Welcome view when the python package file indicated in the current selected configuration does not exist
  2. A different welcome view when the python package file indicated in the current selected configuration is empty of file entries
  3. Another welcome view when python is not part of the project (where the python section is missing in the configuration file).
  4. listing of python packages from the python package file indicated in the current selected configuration when it does have files
  5. updates to the packages listed when the configuration selection changes or if the selected configuration file's package file is changed to another file or if the package file itself is changed.
  6. scan button to overwrite the contents of the package file pointed at by the currently selected configuration
  7. edit button to open the package file pointed at by the currently selected configuration
  8. refresh button to manually force an update in case the file system watchers don't catch it.

Automated Tests

Directions for Reviewers

Checklist

@sagerb sagerb self-assigned this May 1, 2024
@sagerb sagerb marked this pull request as ready for review May 1, 2024 21:52
Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

I was getting some errors when messing around with this without a Python section. I think we need to be more robust and considered with our Python Packages section (not just the webview one, but generally).

@sagerb sagerb requested a review from dotNomad May 2, 2024 23:33
@sagerb
Copy link
Collaborator Author

sagerb commented May 2, 2024

I was getting some errors when messing around with this without a Python section. I think we need to be more robust and considered with our Python Packages section (not just the webview one, but generally).

I've reworked the code around this and believe I've addressed your concerns.

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

Caught some bits that have some odd typing, a if statement that looked a bit odd, and a larger question about how to handle a consistent error on configuration save.

Marking as approved since everything is minor besides the error, and the error can be a follow-up. We can discuss tomorrow morning how to deal with the error.

Comment on lines +435 to +439
const currentConfig = this.getConfigByName(activeConfiguration);
const pythonSection = currentConfig?.configuration.python;
if (!pythonSection) {
pythonProject = false;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay this is very odd. I'm getting an 500 error on the API call below this every time I have a Python section, comment it out, then save.

However throwing a breakpoint in I get 3 calls to _onRefreshPythonPackages(). The first two have the old data, and the final one has the correct data.

I tried opening the file in vim and making a change and I got 6 calls. Opening in nano got me back to 3 calls on save.

I'm unsure how exactly to handle this. It feels like we should only have one event, perhaps we need to audit how the "activeConfigChanged" even is getting triggered and ensure that we aren't getting multiple from a single change.


Since this works, but just throws a notification at the user I'd be comfortable looking at this as a follow-up that we address soon-ish.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Created #1509


const fileUri = Uri.joinPath(this.root.uri, relPathPackageFile);

if (await fileExists(fileUri)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really like this usage here. I forgot we have access to that.

@sagerb sagerb merged commit 3e9c472 into main May 3, 2024
16 checks passed
@sagerb sagerb deleted the sagerb-python-package-webviewview branch May 3, 2024 19:28
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.

Add actions to Requirements webview component Create Requirements webview component
2 participants