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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration from Bower #272

Merged
merged 8 commits into from Jul 27, 2019

Conversation

@jonathanlking
Copy link
Contributor

commented Jun 16, 2019

Description of the change

This (when completed) addresses #159.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 馃槉

jonathanlking added some commits Jun 15, 2019

@jonathanlking

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

This was originally intended as a command to verify that package set satisfies bower dependencies, however after speaking to @f-f I was made aware of #159. I'm going to refactor it to run on spago init and add the dependencies to the spago.dhall.

@f-f

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

@jonathanlking I think we're done with the invasive changes to Bower-related code after #324, would you still like to work on this? If yes then we can rebase on #324, if not I can pick this up as it's needed for 1.0 anyways 馃檪

Also no worries about fixing #159 here, I'd be fine merging pretty much what you have here already (except we should remove the command line stuff) and tackling the rest in a later PR - the policy is that I'm fine even merging code that is not used anywhere yet if it's needed as a stepping stone for other things, as long as we don't change the CLI interface

@jonathanlking

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

@f-f Thanks for letting me know 馃槃
I think your suggestion about removing the command line interface an merging sounds good. I still think there is some value in this check being "continuously" run, rather than just at conversion - given that we need to keep a bower.json file around for Pursuit.
I intend to rebase and remove the command line interface. I had forgotten about local packages when implementing this, so they are currently unsupported.

@f-f

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

@jonathanlking thank you! A few notes:

  • note that when using spago bump-version (implemented, shipping in next version) and spago publish (to be implemented soon) the bower.json is generated from the spago.dhall, and not viceversa. I.e. this conversion will need to happen only when porting a bower package to Spago, and after that Spago will take care of keeping bower file consistent over time
  • no worries about local packages, we'll fix them in the process of taking this code into use (also there's #301 that shuffles the local packages a bit, so it's ok to not worry about them here)
@f-f

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@jonathanlking I think if you merge jonathanlking#1 then it will pass CI

@f-f f-f marked this pull request as ready for review Jul 27, 2019

@jonathanlking

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

@f-f thanks 馃檪 - I've merged it and fixed the warnings.

@f-f

f-f approved these changes Jul 27, 2019

Copy link
Member

left a comment

Thank you @jonathanlking! 馃檪

@f-f f-f merged commit bbc425e into spacchetti:master Jul 27, 2019

3 checks passed

Summary 1 rule matches and 1 potential rule
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@f-f f-f referenced this pull request Jul 29, 2019

Merged

Add bower migration #342

2 of 2 tasks complete

f-f added a commit that referenced this pull request Jul 30, 2019

Add bower migration (#342)
This reworks the prototype from #272 to provide a smooth migration
from Bower when doing `spago init`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.