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

[WIP] Godot 4 #13

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

[WIP] Godot 4 #13

wants to merge 11 commits into from

Conversation

you-win
Copy link
Member

@you-win you-win commented Mar 1, 2023

Core

  • Status
  • Update
  • Purge
  • Clean

GUI is still extremely unfinished.

@you-win you-win requested a review from bend-n March 1, 2023 04:21
@@ -1,14 +1,14 @@
# Godot Package Manager plugin for godot
# Godot Package Manager plugin for Godot 4
Copy link
Member

Choose a reason for hiding this comment

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

when do we want to merge this, and how do we make the 3.x version usable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Godot 4.0 is releasing soon, so we would want to release GPM as soon as possible. The 3.x version can stay as is as long as it's still usable.

Copy link
Member

Choose a reason for hiding this comment

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

But, like, do we make a 3.x branch or just make a release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Spliting into a 3.x branch and master + a release makes sense

addons/godot-package-manager/gui/main.gd Show resolved Hide resolved
addons/godot-package-manager/gui/npm_search.gd Outdated Show resolved Hide resolved

## Get the name without the scope. Necessary for creating directories since the scope
## can contain an "@" character.
func unscoped_name() -> String:
Copy link
Member

Choose a reason for hiding this comment

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

i think we should remove this, i removed this for the cli, and npm doesnt do this.
having scope be a directory is totally fine.

##
## Returns: [br]
## [param Dictionary] - The response parsed into a [Dictionary].
static func post_request(
Copy link
Member

Choose a reason for hiding this comment

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

when is this ever used?

Copy link
Member Author

Choose a reason for hiding this comment

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

eventually for npm search (I think)

Copy link
Member

Choose a reason for hiding this comment

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

docs say its get request based.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I'll remove it if it's not being used before the stable release. I'm sure I can find a use for it though

Comment on lines +69 to +75
static func get_tarball_url(package_name: String, version: String) -> String:
var response := await get_manifest(package_name, version)
if response.is_empty():
printerr("get_tarball_url response was empty")
return ""

return response.get("dist", {}).get("tarball", "")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static func get_tarball_url(package_name: String, version: String) -> String:
var response := await get_manifest(package_name, version)
if response.is_empty():
printerr("get_tarball_url response was empty")
return ""
return response.get("dist", {}).get("tarball", "")
static func get_tarball_url(package_name: String, unscoped_package_name, package_version: String) -> String:
return "%s/%s/-/%s-%s.tgz" % [REGISTRY, package_name, unscoped_package_name, package_version]
# https://registry.npmjs.org/@bendn/splitter/-/splitter-1.0.6.tgz

I wonder if this would work

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe as an optimization to remove 1 GET request.

Could split it into:
get_tarball_url_blind and get_tarball_url

Copy link
Member

Choose a reason for hiding this comment

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

the docs say usually in the form of https://registry.npmjs.org/<name>/-/<name>-<version>.tgz... so i guess its not reliable? hmm.


## Default headers to use when sending requests.
const HEADERS := [
"User-Agent: GodotPackageManager/1.0 (godot-package-manager on GitHub)",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"User-Agent: GodotPackageManager/1.0 (godot-package-manager on GitHub)",
"User-Agent: GodotPackageManager/0.1.0 (godot-package-manager on GitHub)",

plugin.cfg says its 0.1.0

Copy link
Member Author

Choose a reason for hiding this comment

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

User agents don't usually use 3 numbers for the first identifier. example

Copy link
Member

Choose a reason for hiding this comment

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

I wasnt aware there was a standard. Whats wrong with ditching the standard though?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can since everyone else just uses the same mozilla user agent anyways

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.

2 participants