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

Add more info to CONTRIBUTING #1259

Merged
merged 4 commits into from
Oct 21, 2015
Merged

Add more info to CONTRIBUTING #1259

merged 4 commits into from
Oct 21, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Oct 9, 2015

cc @rails-api/ams

needs a second set of eyes and some reworking

@bf4
Copy link
Member Author

bf4 commented Oct 9, 2015

I'm thinking of maybe adding some stuff from https://github.com/bundler/bundler/blob/d79615ab5d13ca3c7d174f45eabdcd6ada717658/lib/bundler/friendly_errors.rb#L49-L71

   Bundler.ui.info <<-EOS.gsub(/^ {6}/, "")
      --- ERROR REPORT TEMPLATE -------------------------------------------------------
      - What did you do?
        I ran the command `#{$PROGRAM_NAME} #{ARGV.join(" ")}`
      - What did you expect to happen?
        I expected Bundler to...
      - What happened instead?
        Instead, what actually happened was...
      Error details
          #{e.class}: #{e.message}
            #{e.backtrace.join("\n            ")}
      #{Bundler::Env.new.report(:print_gemfile => false, :print_gemspecs => false).gsub(/\n/, "\n      ").strip}
      --- TEMPLATE END ----------------------------------------------------------------
    EOS

   Bundler.ui.error "Unfortunately, an unexpected error occurred, and Bundler cannot continue."

    Bundler.ui.warn <<-EOS.gsub(/^ {6}/, "")
      First, try this link to see if there are any existing issue reports for this error:
      #{issues_url(e)}
      If there aren't any reports for this error yet, please create copy and paste the report template above into a new issue. Don't forget to anonymize any private data! The new issue form is located at:
      https://github.com/bundler/bundler/issues/new
    EOS
  end

  def self.issues_url(exception)
    "https://github.com/bundler/bundler/search?q=" \
    "#{CGI.escape(exception.message.lines.first.chomp)}&type=Issues"
  end


- Using `grape-active_model_serializers`, or any non-Rails server. See
[issue](https://github.com/rails-api/active_model_serializers/issues/1258).
-
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably belongs in a FAQ no?

@beauby
Copy link
Contributor

beauby commented Oct 9, 2015

👍 Some minor comments. Awesome otherwise!

@bf4 bf4 force-pushed the creating_an_issue branch 2 times, most recently from 05ea02a to 5b09fb7 Compare October 9, 2015 20:00
@bf4
Copy link
Member Author

bf4 commented Oct 9, 2015


## Common issues and resolutions

- Using `grape-active_model_serializers`, or any non-Rails server. See
Copy link
Member

Choose a reason for hiding this comment

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

👍

@joaomdmoura
Copy link
Member

LGTM! 👍 just 2 small comments

@bf4 bf4 force-pushed the creating_an_issue branch from 5b09fb7 to 285dd6d Compare October 14, 2015 22:42
@bf4
Copy link
Member Author

bf4 commented Oct 14, 2015

updated

Everyone is encouraged to open issues that are affecting you: bugs, ideas, performance problems – everything helps!
### Filing an issue

Everyone is encouraged to open issues that are affecting you:
Copy link
Contributor

Choose a reason for hiding this comment

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

Problem with grammar here (everyone...you).

Copy link
Member Author

Choose a reason for hiding this comment

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

wow, you're on fire! 🔥

Copy link
Member Author

Choose a reason for hiding this comment

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

but I'm going to write Everyone is encouraged to open issues that are affecting them:


1. The first place to start is by looking at our [GitHub Issues](https://github.com/rails-api/active_model_serializers/issues).

- Check if your issue has already reported.
Copy link
Contributor

Choose a reason for hiding this comment

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

has already been reported

@bf4
Copy link
Member Author

bf4 commented Oct 15, 2015

Added edits in new commit for ease of review

- What did you expect to happen?
- What happened? Include as much information as possible.
- Nature of reported defect (e.g. user name missing, not "It doesn't work."). Is it intermittent?
- The best help here is a failing test.
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be clarified that the failing test should be a PR to the AMS repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

not always, but yeah

@bf4 bf4 force-pushed the creating_an_issue branch from ab1504b to 91c5450 Compare October 16, 2015 04:38
@bf4 bf4 force-pushed the creating_an_issue branch from 91c5450 to 8c52c36 Compare October 16, 2015 04:39
@joaomdmoura
Copy link
Member

Just one comment abotu the docs, other then this LGTM

@bf4
Copy link
Member Author

bf4 commented Oct 21, 2015

@joaomdmoura your comment seemed more appropriate to the PR section, so I put it there

@bf4 bf4 force-pushed the creating_an_issue branch from a8b5846 to fcf5f8c Compare October 21, 2015 20:44
@joaomdmoura
Copy link
Member

Yup @bf4, I agree. Never meant for you put it right in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants