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

feat!: replace StopMagic with autocompleteHeaders #136

Merged
merged 5 commits into from
Jun 27, 2023
Merged

feat!: replace StopMagic with autocompleteHeaders #136

merged 5 commits into from
Jun 27, 2023

Conversation

theseion
Copy link
Collaborator

StopMagic is still supported for backwards compatibility.

@theseion theseion marked this pull request as draft February 26, 2023 16:19
@fzipi
Copy link
Member

fzipi commented Feb 26, 2023

Ugh, maybe AutoCompleteHeaders being true as the default (like now), and then setting it to false?

@theseion
Copy link
Collaborator Author

That was my first impulse. However, there are no other fields that use values that differ from the type default yet, so I would have to switch to using constructors instead of Input{}. Might be a good idea, but I wanted to keep the PR small.

@theseion theseion requested a review from fzipi April 2, 2023 12:30
@theseion theseion marked this pull request as ready for review April 2, 2023 12:30
@theseion theseion changed the title feat: replace StopMagic with NoAutocompleteHeaders feat!: replace StopMagic with NoAutocompleteHeaders Apr 2, 2023
fzipi
fzipi reviewed Apr 2, 2023
View reviewed changes
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Change looks good. But, we cannot merge this removing stop_magic now.

Once we have version 1.1 or 2.0 of the test spec, maybe.

@theseion
Copy link
Collaborator Author

theseion commented Apr 2, 2023

Ok. I'll rework it to retain stop_magic as legacy option.

@fzipi
Copy link
Member

fzipi commented May 24, 2023

@theseion How is that rework going on? Need help on this?

@theseion
Copy link
Collaborator Author

If you want to, sure. I'm not getting much done at the moment.

@theseion theseion force-pushed the replace-stop-magic branch 2 times, most recently from c004ee7 to 8a78f33 Compare June 24, 2023 17:30
@theseion theseion changed the title feat!: replace StopMagic with NoAutocompleteHeaders feat!: replace StopMagic with autocompleteHeaders Jun 24, 2023
@theseion
Copy link
Collaborator Author

I've rewritten the PR to use *bool for all values in Input and Overrides. That makes it possible to detect missing values. Because of that I've also been able to change the name (and inversen the logic) to AutocompleteHeaders. StopMagic still works as before but AutocompleteHeaders has precedence.

@theseion theseion requested a review from fzipi June 24, 2023 17:38
@theseion theseion mentioned this pull request Jun 25, 2023
Merged
fzipi
fzipi reviewed Jun 26, 2023
View reviewed changes
test/types.go Show resolved Hide resolved
@theseion
feat: replace StopMagic with NoAutocompleteHeaders
eb077bf 
StopMagic is still supported for backwards compatibility.
@theseion
feat!: replace StopMagic with NoAutocompleteHeaders
7c68807 
BREAKING CHANGE: clients will need to replace `stop_magic` in their test
files with `no_autocomplete_headers`.
@theseion
wip: working post processing logic
a02976c
@theseion
wip: use *bool to detect missing values
fe0a5e0 
- use *bool instead of bool for all value of Input and Overrides
- centralise post loading / override application
- add / adapt tests
@theseion
chore: change default value and name to AutoCompleteHeaders
2a47d69
@fzipi fzipi merged commit b08d5da into coreruleset:main Jun 27, 2023
3 checks passed
@theseion theseion deleted the replace-stop-magic branch June 27, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzipi fzipi fzipi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@theseion @fzipi