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

Only check for @generated-like markers in file header #2654

Merged
merged 3 commits into from
Oct 14, 2023

Conversation

sanmai-NL
Copy link
Contributor

@sanmai-NL sanmai-NL commented May 18, 2023

Check a large but limited buffer from the start of the file. Do not assume UTF-8 encoding and text decode at all, but search for byte string. First check for the marker with the highest priority, to short-circuit return when it is found.

This prevents performance problems with large files.

Proposed Changes

  1. Small, adequately described in first commit message.

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@nvuillam
Copy link
Member

@sanmai-NL that's a brilliant idea for perfs :)

Please can you also update the CHANGELOG.md ?

@sanmai-NL
Copy link
Contributor Author

@nvuillam How to add labels?

@nvuillam
Copy link
Member

done ^^

megalinter/utils.py Outdated Show resolved Hide resolved
@sanmai-NL
Copy link
Contributor Author

sanmai-NL commented May 18, 2023

I examined the docs, and I think they needn't be changed, likewise, I don't consider this change breaking for spec-compliant data. Before the marker was sought anywhere in the source file, now it is only in the beginning. The latter is specified by the spec. ‘The beginning’ is not an objective criterion. Hopefully the original spec can be updated to define a limit.

@sanmai-NL sanmai-NL marked this pull request as ready for review May 18, 2023 12:47
@sanmai-NL sanmai-NL requested a review from nvuillam as a code owner May 18, 2023 12:47
@nvuillam
Copy link
Member

@sanmai-NL works for me :)

@sanmai-NL
Copy link
Contributor Author

@nvuillam Do you have chance to review this small but impactful change soon? Can it be included in v7?

@nvuillam
Copy link
Member

@sanmai-NL it will be in 7.1.0 , sorry v7 has just been released :)

Please can you check our comments with @Kurt-von-Laven about readability of the condition ?

@sanmai-NL sanmai-NL temporarily deployed to dev June 5, 2023 09:32 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jul 6, 2023
@nvuillam
Copy link
Member

nvuillam commented Jul 6, 2023

@sanmai-NL friendly ping ? :)

@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jul 6, 2023
@sanmai-NL
Copy link
Contributor Author

I don't see unaddressed comments.

@sanmai-NL sanmai-NL temporarily deployed to dev July 7, 2023 20:54 — with GitHub Actions Inactive
@nvuillam
Copy link
Member

@sanmai-NL please can you add SOURCEFILEHEADER in cspell.json so the CI pass ?

I also think that there is some issue with the git delta in CHANGELOG.md

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 10, 2023
@sanmai-NL
Copy link
Contributor Author

@nvuillam I don't know exactly the what and why of your instructions. Can you make the required tweaks to this branch?

megalinter/utils.py Show resolved Hide resolved
megalinter/utils.py Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 16, 2023
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Sep 15, 2023
@github-actions github-actions bot closed this Sep 29, 2023
@sanmai-NL
Copy link
Contributor Author

/remove stale

@sanmai-NL
Copy link
Contributor Author

@nvuillam

@echoix echoix added nostale This issue or pull request is not stale, keep it open and removed O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity labels Sep 29, 2023
@nvuillam nvuillam reopened this Oct 1, 2023
@nvuillam nvuillam temporarily deployed to dev October 1, 2023 23:47 — with GitHub Actions Inactive
@nvuillam
Copy link
Member

nvuillam commented Oct 1, 2023

@sanmai-NL you need to merge conflicts to make your branch up to date :)

Check a large but limited buffer from the start of the file.
Do not assume UTF-8 encoding and text decode at all, but search for byte string.
First check for the marker with the highest priority, to short-circuit return when it is found.

This prevents performance problems with large files
@sanmai-NL
Copy link
Contributor Author

@sanmai-NL you need to merge conflicts to make your branch up to date :)

@nvuillam Ready to merge.

@sanmai-NL sanmai-NL temporarily deployed to dev October 14, 2023 09:35 — with GitHub Actions Inactive
Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution that should improve performances :)
Once CI is ok i'll merge :)

@nvuillam nvuillam temporarily deployed to dev October 14, 2023 10:15 — with GitHub Actions Inactive
@nvuillam nvuillam temporarily deployed to dev October 14, 2023 10:15 — with GitHub Actions Inactive
@nvuillam nvuillam merged commit 4f5dcd5 into oxsecurity:main Oct 14, 2023
7 checks passed
@sanmai-NL sanmai-NL deleted the patch-2 branch December 6, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nostale This issue or pull request is not stale, keep it open performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants