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

Inform user of missing properties on publish #815

Closed
wants to merge 2 commits into from

Conversation

aeisenberg
Copy link
Contributor

Initial fix for issue #694

Simple prompt for asking users to create a better bower.json.

@satazor
Copy link
Member

satazor commented Aug 23, 2013

@aeisenberg I was thinking in being more aggressive like npm is. npm warns when a package is fetched, for every user. This makes users to report the issue directly to the author. //cc @sindresorhus

@sindresorhus
Copy link
Contributor

@satazor Huge 👍

Authors are incredible lazy and that's really the only way. With npm users did a lot of PRs to fix the situation which was great. Though I think we should do both, as the author should get a chance to fix it.

@satazor
Copy link
Member

satazor commented Aug 23, 2013

@sindresorhus not necessary, since register fetches the package to ensure it's valid. So it will make validation anyway. @aeisenberg are you willing to update PR? This is the right place to do it: https://github.com/bower/bower/blob/master/lib/core/resolvers/Resolver.js#L210

Some tests would also be good.

@aeisenberg
Copy link
Contributor Author

That's a great idea. I will update the PR with another commit that nags
people installing the package as well. Thanks for the pointer on where to
look.

On Fri, Aug 23, 2013 at 3:08 PM, André Cruz notifications@github.comwrote:

@sindresorhus https://github.com/sindresorhus not necessary, since
register fetches the package to ensure it's valid. So it will make
validation anyway. @aeisenberg https://github.com/aeisenberg are you
willing to update PR? This is the right place to do it:
https://github.com/bower/bower/blob/master/lib/core/resolvers/Resolver.js#L210

Some tests would also be good.


Reply to this email directly or view it on GitHubhttps://github.com//pull/815#issuecomment-23194291
.

@aeisenberg
Copy link
Contributor Author

OK. Updated the pull request to warn users who install packages with missing properties. I kept the original change as well to warn users on publish.

Right now, only warning for the main and ignore properties, but it is easy to extend for more, and possible to add more sophisticated checks. A test case is included.

I squashed the new changes into the previous commit, so you will have to do a force pull to get the latest without duplicating commits.

@aeisenberg
Copy link
Contributor Author

As soon as I sent this, it occurs to me that what I have isn't ideal. Now, the checking logic is duplicated in the publish and install areas. It would be best to pull this out and create some sort of validateMetadata function where everything happens in one place. If validation logic becomes more complicated, having one place where this happens will be essential, but I'll leave it up to you to see if this is a requirement for this PR.

@satazor
Copy link
Member

satazor commented Aug 25, 2013

@aeisenberg as I said early, there's no necessity to do it in the register command, since it will fetch the package to ensure it's valid before registering. https://github.com/bower/bower/blob/master/lib/commands/register.js#L42

So there's no necessity for duplicate code, you can completely rollback the register changes you made.

@aeisenberg
Copy link
Contributor Author

OK. Another change to pull request. remove the check on register. Now only check during install.

@sheerun
Copy link
Contributor

sheerun commented Apr 11, 2014

@satazor We do it in http://rails-assets.org for missing main files :) What's the status of this?

@sheerun
Copy link
Contributor

sheerun commented Jun 8, 2014

Scheduling on 1.4.0. Who wants to review?

@sheerun sheerun added this to the 1.4.0 milestone Jun 8, 2014
@sheerun sheerun modified the milestones: 1.3.x, 1.4.x Jun 20, 2014
@sheerun
Copy link
Contributor

sheerun commented Jun 20, 2014

Commited in 1690dd4

@sheerun
Copy link
Contributor

sheerun commented Jun 20, 2014

Tracking progress moved to #694

@sheerun sheerun closed this Jun 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants