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.

Sign up for GitHub

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

Jump to bottom

Add lint for debug impls on exported types #2266

Closed
sffc opened this issue Jul 27, 2022 · 3 comments · Fixed by #3206
Closed

Add lint for debug impls on exported types #2266

sffc opened this issue Jul 27, 2022 · 3 comments · Fixed by #3206
Assignees
@zbraniecki
Labels
C-meta Component: Relating to ICU4X as a whole S-medium Size: Less than a week (larger bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt
Milestone
1.2 Blocking ⟨P1⟩

Comments

@sffc
Copy link
Member

sffc commented Jul 27, 2022

Context: #2250 (comment)

Our style guide says that we should implement Debug on all exported types; however, we don't currently do that.

@robertbastian suggested the missing_debug_implementations rustc lint.

@Manishearth thoughts?

Most likely not time for 1.0 but we should try to land it in 1.1
@sffc sffc added C-meta Component: Relating to ICU4X as a whole T-techdebt Type: ICU4X code health and tech debt S-medium Size: Less than a week (larger bug fix or enhancement) needs-approval One or more stakeholders need to approve proposal labels Jul 27, 2022
@sffc sffc added this to the ICU4X 1.1 milestone Jul 27, 2022
@robertbastian robertbastian self-assigned this Jul 27, 2022
@Manishearth
Copy link
Member

Yeah, seems like a good idea!

@robertbastian robertbastian mentioned this issue Jul 27, 2022
Merged
@sffc sffc removed the needs-approval One or more stakeholders need to approve proposal label Aug 11, 2022
@sffc sffc modified the milestones: ICU4X 1.1, ICU4X 1.2 Dec 22, 2022
@robertbastian
Copy link
Member

Is this 1.2 blocking?

@sffc
Copy link
Member Author

sffc commented Mar 16, 2023

  • @sffc - Is this a property we want to retain in the style guide?
  • @Manishearth - I think it's helpful when things have debug impls, but the debug impls don't necesarily need to be useful. But what it helps is that derive(Debug) can work.
  • @nordzilla - I strongly prefer it when everything has a Debug impl.

@sffc sffc assigned zbraniecki and unassigned robertbastian Mar 16, 2023
@zbraniecki zbraniecki mentioned this issue Mar 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zbraniecki zbraniecki

Labels
C-meta Component: Relating to ICU4X as a whole S-medium Size: Less than a week (larger bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt
Projects
None yet
Milestone
1.2 Blocking ⟨P1⟩
Development

Successfully merging a pull request may close this issue.

Add missing Debug impls zbraniecki/icu4x
4 participants
@zbraniecki @sffc @Manishearth @robertbastian