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

[core] Cleanup issues body #10372

Merged
merged 1 commit into from
Sep 17, 2023
Merged

[core] Cleanup issues body #10372

merged 1 commit into from
Sep 17, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Sep 17, 2023

Closes #10340

Remove the checkboxes from new issues' body. I have hardcoded the strings, seems good enough.

Result: https://github.com/romgrk/mui-test-issue-cleanup/issues/3

@romgrk romgrk added the core Infrastructure work going on behind the scenes label Sep 17, 2023
@romgrk romgrk requested review from Janpot and LukasTy September 17, 2023 13:48
});

const text =
'### Duplicates\r\n\r\n- [X] I have searched the existing issues\r\n\r\n### Latest version\r\n\r\n- [X] I have tested the latest version\r\n\r\n';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Github seems to store newlines using windows conventions.

@mui-bot
Copy link

mui-bot commented Sep 17, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-10372--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms -212.3 67.7 -212.3 -104.7 98.986
Sort 100k rows ms 719.4 1,398.5 1,310.6 1,187.92 241.02
Select 100k rows ms 618.1 767.3 683.9 681.68 51.151
Deselect 100k rows ms 121.9 242.1 158.8 171.78 42.85

Generated by 🚫 dangerJS against b435ed1

@Janpot
Copy link
Member

Janpot commented Sep 17, 2023

I have hardcoded the strings, seems good enough.

Would it work if we add

  - type: markdown
    attributes:
      value: <!-- delete:start -->

  # checkboxes ...

  - type: markdown
    attributes:
      value: <!-- delete:end -->

And postprocess them with

const body = issue.data.body
  .replaceAll('<!-- delete:start -->', '<!--')
  .replaceAll('<!-- delete:end -->', '-->')

Doesn't seem much more complicated, it doesn't have to be kept in sync with the template strings, and directly reusable in other templates.

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Works for me. Commented with an alternative implementation. But fine to merge as is as well.

@romgrk
Copy link
Contributor Author

romgrk commented Sep 17, 2023

That's a neat idea, but we often edit issues main comment along the way, and leaving those comments in there would still be a bit annoying. But it's trivial to update in the future so I'll merge as is for now.

@romgrk romgrk merged commit 25d175c into mui:master Sep 17, 2023
@romgrk romgrk deleted the core-issue-cleanup branch September 17, 2023 15:18
@Janpot
Copy link
Member

Janpot commented Sep 17, 2023

That's a neat idea, but we often edit issues main comment along the way, and leaving those comments in there would still be a bit annoying.

Perhaps then we could do

const body = issue.data.body.replaceAll(/<!-- delete:start -->(\n|.)*?<!-- delete:end -->/g, '')

?

I think we still should combine the headers. And did you give this some thought? My personal feeling is that the solution in this PR is the least favourable of the shaping page.

@oliviertassinari oliviertassinari added the scope: code-infra Specific to the core-infra product label Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Improve GitHub template
4 participants