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] Add support for missing README files and other README file extensions #420

Closed
wants to merge 1 commit into from

Conversation

mstallmo
Copy link
Member

Based on feedback from @fitzgen on #411 I went back and updated the copying of the README.md file to propagate the error as well if one occurs. I had referenced that functionality in writing the license copy so I figured it would be good to go back and update that one as well.


Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

@mstallmo
Copy link
Member Author

Build is failing due to cargo fmt but it looks like that's coming from master. I fixed the formatting in #411 but figured I should hold off doing it here again to avoid things getting too confusing.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

In this case, we should probably check whether the file exists or not, or else if there is no README.md file at all, we will return an error, which shouldn't be the case. So I don't think we want this particular change. Does this pass tests? If so, we should have a test for building without a README.md file.

This also made me realize that we don't handle arbitrary README files, so I filed #421

@mstallmo
Copy link
Member Author

It does pass the current suite of tests and I agree it would be good to have tests for a README not existing. If it's cool with everyone I can just update this pull request to satisfy #421 since the description sounds similar to the implementation for #411.

@fitzgen fitzgen changed the title Propagate errors when copying README.md [WIP] Add support for missing README files and other README file extensions Oct 29, 2018
@fitzgen
Copy link
Member

fitzgen commented Oct 29, 2018

Sure thing -- sounds good to me! I updated the title of the PR to reflect this change in direction.

@fitzgen fitzgen added the work in progress do not merge! label Oct 29, 2018
@fitzgen fitzgen added this to the 0.6.0 milestone Oct 29, 2018
@ashleygwilliams
Copy link
Member

one thing to share- we should not error if the readme is missing. only warn.

@ashleygwilliams
Copy link
Member

After reading the associated issue I want to warn that npm does not support these Readme files so i'm not entirely sure that we should? I am curious why you thought this was a bug @fitzgen

@fitzgen
Copy link
Member

fitzgen commented Nov 9, 2018

I had no idea npm doesn't support non-md READMEs! This was motivated by github's support for other kinds of READMEs, which means that some projects use that support. It is perhaps worth reconsidering if npm doesn't allow it, though!

@mstallmo
Copy link
Member Author

Sorry for the delay but I have everything ready to be pushed up and reviewed now. Is this something we still want or should we close this for now since NPM doesn't support anything except for .md files?

@ashleygwilliams
Copy link
Member

for the moment i think we should close this as npm doesn't support other README file extensions. if there's a usecase for supporting other README file extensions, and a way to make sure folks aren't confused about npm not supporting the other formats, i'm open to hearing it!

@ashleygwilliams ashleygwilliams removed this from the 0.6.0 milestone Jan 11, 2019
@ashleygwilliams ashleygwilliams removed the work in progress do not merge! label Jan 11, 2019
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.

3 participants