Skip to content

Make report mod public - #56

Merged
spacekookie merged 2 commits into
rust-cli:masterfrom
passy:public-report
Jun 15, 2019
Merged

spacekookie merged 2 commits into
rust-cli:masterfrom
passy:public-report

Conversation

@passy

@passy passy commented Apr 18, 2019

Copy link
Copy Markdown
Contributor

Choose one: 🙋 feature

Checklist

  • tests pass

Context

Hi! 👋 I was wondering if you'd be opposed to the idea of exposing the report mod as public. It is pretty useful on its own and I intend to use the Report struct directly instead of printing the result.

No worries if you don't want to increase the API surface, though. :)

Semver Changes

It changes the public interface, but in a backwards-compatible way. So minor bump should be fine.

@spacekookie

Copy link
Copy Markdown
Collaborator

Hey there! Sorry it took so long to get to this.

So generally I'm not opposed to this. Especially because making human-panic more pluggable has been on my agenda for a while.
There was a PR a while ago that kinda added window handling for panics which was a pretty neat idea but came with some other drawbacks. But this spawned the idea of having a general interface for letting programs deal with reports in some way (whether that just write to disk or be an automatic crash reporter or whatever).

Making this mod pub would go a good way towards allowing this. But I'm wondering if there's a nicer way to achieve that goal and if you'd be interested in collaborating to this effort.

Anyway, if we end up just making this module public there needs to be documentation for the newly exposed functions. I defer further opinions on this matter to @killercup and @yoshuawuyts 👋

@yoshuawuyts

Copy link
Copy Markdown
Collaborator

I have no strong opinions here; happy to defer to your judgement @spacekookie!

@passy

passy commented May 6, 2019

Copy link
Copy Markdown
Contributor Author

Hi! Now I have to apoligise for the long delay. I really like the approach of trying to find a more generic solution to the problem of working with reports. I'd definitely be happy to help out with this, whether it's by adding the missing docs or a different approach if you have something in mind. :)

@spacekookie spacekookie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey @passy, I've started doing some work on v2.0.0 of this crate and would like to merge your changes.

But CI is currently failing. Could you make sure that all items from the report module are properly documented :)

@passy

passy commented Jun 15, 2019

Copy link
Copy Markdown
Contributor Author

@spacekookie Amazing! I added some docstrings. Can you let me know if this looks good to you? I'm happy to expand or change things.

@spacekookie spacekookie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Excellent, looks good to me :)

@spacekookie
spacekookie merged commit 200e427 into rust-cli:master Jun 15, 2019
epage added a commit that referenced this pull request Sep 12, 2026
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [j178/prek-action](https://redirect.github.com/j178/prek-action) |
action | major | `v1` → `v2` |

---

### Release Notes

<details>
<summary>j178/prek-action (j178/prek-action)</summary>

### [`v2`](https://redirect.github.com/j178/prek-action/compare/v1...v2)

[Compare
Source](https://redirect.github.com/j178/prek-action/compare/v1...v2)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 5am on the first day of the
month" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/epage/_rust).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDAuMCIsInVwZGF0ZWRJblZlciI6IjQzLjEwMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
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