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
Merged

Conversation

jonathanlking
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

f-f 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
Copy link
Contributor Author

@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
Copy link
Member

f-f 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 Import local packages as Location聽#301 that shuffles the local packages a bit, so it's ok to not worry about them here)

@f-f
Copy link
Member

f-f 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 July 27, 2019 15:25
@jonathanlking
Copy link
Contributor Author

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

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

Thank you @jonathanlking! 馃檪

@f-f f-f merged commit bbc425e into purescript:master Jul 27, 2019
@f-f f-f mentioned this pull request Jul 29, 2019
2 tasks
f-f added a commit that referenced this pull request Jul 30, 2019
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants