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 config option generated_marker_line_search_limit #5993

Merged
merged 3 commits into from
Jan 20, 2024

Conversation

IVIURRAY
Copy link
Contributor

@IVIURRAY IVIURRAY commented Dec 28, 2023

Fixes #5658 ...and a follow on of the work in #5659 by @captbaritone which has not been updated in a while.

FYI @ytmimi - I noticed your comments on the previous PR and address them in this PR. Mainly around the docs changes and adding more tests.

p.s. - learning Rust so apologies for any basic errors here.

@IVIURRAY IVIURRAY changed the title Add config option generated_file_header_size #5659 Add config option generated_file_header_size Dec 28, 2023
@IVIURRAY IVIURRAY changed the title Add config option generated_file_header_size Add config option generated_file_header_size Dec 28, 2023
@ytmimi
Copy link
Contributor

ytmimi commented Dec 28, 2023

@IVIURRAY thanks for taking the time to work on this and make those updates. As Caleb mentioned in #5659 (comment), he'd like to use a more descriptive name for this option. Do you have any ideas for a succinct and clear name that better convey the intent of this option?

One idea I had was calling it generated_file_marker_search_lines. It's a little wordy, so let me know if you can come up with something better.

@IVIURRAY
Copy link
Contributor Author

IVIURRAY commented Dec 28, 2023

I'm not a fan of generated_file_marker_search_lines as it indicates you're seaching for them when really is it just taking the first N lines. So really a header or prefix would be a better descriptor, in my opinion.

I used chatGPT (and a few in myself) to generate us some ideas...my favourites being:

  1. file_prefix_lines or generated_prefix_lines or generated_prefix_size
  2. file_auto_generated_lines or file_auto_generated_size
  3. file_header_lines or generated_header_lines or generated_header_size
    header_line_count
    file_auto_generated_prefix_lines
    file_prefix_lines
    generated_line_count
    auto_generated_line_count
    header_lines
    file_header_lines
    header_prefix_lines
    copyright_header_lines
    generated_header_lines
    license_header_lines
    meta_header_lines
    source_tree_header_lines
    signed_source_header_lines
    file_metadata_lines
    start_of_file_header_lines
    file_start_header_lines
    initial_header_lines
    top_comment_lines
    header_intro_lines
    file_intro_lines
    code_header_lines
    declaration_header_lines
    file_heading_lines

@captbaritone
Copy link
Contributor

Thanks for picking this up!

@ytmimi
Copy link
Contributor

ytmimi commented Dec 28, 2023

I'm not a fan of generated_file_marker_search_lines as it indicates you're searching for them when really is it just taking the first N lines. So really a header or prefix would be a better descriptor, in my opinion.

@IVIURRAY Just want to make sure we're on the same page. Yes this option will literally configure N so that we can consider the first N lines, but we're doing so in order to search for the @generated marker.

Thanks for the list. I'm sure that will help us come up with the right name for this option.

@IVIURRAY
Copy link
Contributor Author

Oh I see, yes, you're right. It is less of a prefix and more a range. Find the @generated marker within the first N lines.

I'll wait to hear from you on what you decide 😄

@ytmimi
Copy link
Contributor

ytmimi commented Jan 10, 2024

Caleb echoed my thought process when we had our team meeting earlier this week. Here's a link to his comment. Ultimately the name needs to reflect that we're searching for the @generated marker within the first n lines of the file.

@IVIURRAY
Copy link
Contributor Author

IVIURRAY commented Jan 14, 2024

Thanks for the links to the conversation. This has help me understand the context and change in more depth.

Here are a few more suggestions given the context:

  • generated_marker_line_search_limit (This would be my choice as it is the most obvious; I would get a good undersatnding about what it is doing without needing to look at the dcos)
  • generated_tag_line_search_limit
  • generated_marker_search_limit
  • generated_marker_check_limit
  • generated_line_check_limit
  • generated_tag_limit

===== ATTACHING DOCS FROM MR FOR QUICK REFERENCE BELOW =====

generated_file_header_size

Number of lines to check for a @generated pragma header when format_generated_files is false. When format_generated_files is true, this option has no effect.

  • Default value: 5
  • Possible values: any positive integer
  • Stable: No (tracking issue: #5080)

See also format_generated_files link here.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 14, 2024

generated_marker_line_search_limit seems like a reasonable name to me.

@captbaritone
Copy link
Contributor

generated_marker_line_search_limit seems good to me as well.

@IVIURRAY IVIURRAY force-pushed the 5658-generated-file-header-size branch from afc6e58 to 1d7122b Compare January 15, 2024 20:40
@IVIURRAY IVIURRAY changed the title Add config option generated_file_header_size Add config option generated_marker_line_search_limit Jan 15, 2024
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

A few more changes I'd like to see before moving forward.

src/formatting/generated.rs Outdated Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
change config name to generated_marker_line_search_limit

update docs

fix formatting

use config variable

add 0 limit docs

adding zero limit search
@IVIURRAY IVIURRAY force-pushed the 5658-generated-file-header-size branch from b19a6ea to 1ca738e Compare January 16, 2024 20:40
@calebcartwright
Copy link
Member

i like the new generated_marker_line_search_limit name 👍

@IVIURRAY
Copy link
Contributor Author

@ytmimi let me know if you want more changes as I'm considering this MR as done now 🙂

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

@IVIURRAY thank you for driving this one to completion 😁

@ytmimi ytmimi merged commit bf96731 into rust-lang:master Jan 20, 2024
27 checks passed
@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-follow-up-review-pending labels Jan 20, 2024
@IVIURRAY IVIURRAY deleted the 5658-generated-file-header-size branch January 29, 2024 20:57
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Jul 6, 2024
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.

Option to configure how many lines down the @generated tag can be for format_generated_files?
5 participants