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.

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

feat(install): rewrite posh script & opt-in marketplace install #2663

Merged
merged 24 commits into from
Dec 27, 2023
Merged

feat(install): rewrite posh script & opt-in marketplace install #2663

merged 24 commits into from
Dec 27, 2023

Conversation

SunsetTechuila
Copy link
Contributor

test carefully and feel free to suggest changes

@afonsojramos
Copy link
Member

I'm very wary of changing our install scripts at this point in time, because they currently work well, and we'll probably move off of install scripts for v3. If I have access to a windows machine in the next week I'll try to test it out!

@afonsojramos
Copy link
Member

We should probably also update the documentation for the installation then, no?
And also get the .sh installation to also query for Marketplace installation, way better implementation than simply giving two scripts directly for sure.

@SunsetTechuila
Copy link
Contributor Author

SunsetTechuila commented Nov 24, 2023 •
edited
Loading

We should probably also update the documentation for the installation then, no?

yes

And also get the .sh installation to also query for Marketplace installation,

i don't know bash

@SunsetTechuila
Copy link
Contributor Author

SunsetTechuila commented Dec 5, 2023 •
edited
Loading

And also get the .sh installation to also query for Marketplace installation

@afonsojramos done

@afonsojramos
Copy link
Member

@SunsetTechuila is this expected behaviour?
image

@afonsojramos
Copy link
Member

Shell one looks good tho!

@SunsetTechuila
Copy link
Contributor Author

is this expected behaviour?

no, have fixed it

@SunsetTechuila SunsetTechuila changed the title refactor(install/windows): rewrite script feat(install): rewrite posh script & add option to install marketplace Dec 5, 2023
@afonsojramos
Copy link
Member

@SunsetTechuila getting this at the marketplace section
image

@SunsetTechuila
Copy link
Contributor Author

@afonsojramos fixed

image

afonsojramos
afonsojramos requested changes Dec 18, 2023
View reviewed changes
Copy link
Member

@afonsojramos afonsojramos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@SunsetTechuila The only thing that I don't quite like is that most messages show up as "verbose". That doesn't make that much sense if the "Success" afterwards doesn't... Let's keep them as normal messages, like we've had.

image

PS: This was ran in a windows pc that doesn't have Spotify installed.

@SunsetTechuila
Copy link
Contributor Author

SunsetTechuila commented Dec 18, 2023 •
edited
Loading

@afonsojramos is it better now?

image

image

afonsojramos
afonsojramos approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

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

I feel like it is perfect now, thanks @SunsetTechuila! Requesting some other eyes as well

rxri
rxri requested changes Dec 22, 2023
View reviewed changes
Copy link
Member

@rxri rxri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • The install.log is missing, and it was really useful feature, please bring it back
  • Colors are not really nice compared to the older script. Personally, I would switch to the old ones
  • It would be nice to highlight the spicetify-h and change the message containing it because it says [...] to get started, but -h just shows help message so it isn't really clear
  • Also it seems like marketplace script has problem with detecting if spicetify is already applied?
    image

@SunsetTechuila
Copy link
Contributor Author

The install.log is missing, and it was really useful feature, please bring it back

I've never seen anyone use it

@rxri
Copy link
Member

rxri commented Dec 22, 2023

The install.log is missing, and it was really useful feature, please bring it back

I've never seen anyone use it

v3 will definitely use it and that's how we will ask people to send install logs. Removing it now, isn't the best idea then

@SunsetTechuila
Copy link
Contributor Author

v3 will definitely use it

we'll probably move off of install scripts for v3

image

ummm?

@rxri
Copy link
Member

rxri commented Dec 22, 2023

Yes, but the installer will generate the log xd

@SunsetTechuila
Copy link
Contributor Author

and how this makes removing logging that no one uses a bad idea?

@SunsetTechuila
Copy link
Contributor Author

image

4 results. support server exists since 2021

@SunsetTechuila
Copy link
Contributor Author

and change the message containing it

to what exactly

@SunsetTechuila SunsetTechuila requested a review from rxri December 22, 2023 21:09
@theRealPadster theRealPadster removed their request for review December 22, 2023 21:53
rxri
rxri requested changes Dec 27, 2023
View reviewed changes
install.ps1 Outdated Show resolved Hide resolved
install.ps1 Outdated Show resolved Hide resolved
install.ps1 Outdated Show resolved Hide resolved
install.ps1 Outdated Show resolved Hide resolved
install.ps1 Outdated Show resolved Hide resolved
install.ps1 Outdated Show resolved Hide resolved
install.ps1 Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
@rxri rxri changed the title feat(install): rewrite posh script & add option to install marketplace feat(install): rewrite posh script & opt-in marketplace install Dec 27, 2023
@rxri rxri merged commit 5ebf432 into spicetify:master Dec 27, 2023
5 checks passed
@SunsetTechuila SunsetTechuila deleted the posh2 branch December 27, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afonsojramos afonsojramos afonsojramos approved these changes

@rxri rxri rxri approved these changes

@CharlieS1103 CharlieS1103 Awaiting requested review from CharlieS1103

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SunsetTechuila @afonsojramos @rxri