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

Refactor implicit insert issue #3395 #3467

Closed
wants to merge 2 commits into from
Closed

Refactor implicit insert issue #3395 #3467

wants to merge 2 commits into from

Conversation

mostafa630
Copy link

This solution addresses issue #3395 by refactoring the code to use "implicit_insert_type" instead of "insert_type" to make it cleaner.

@P-E-P P-E-P requested a review from philberty March 5, 2025 13:20
@P-E-P P-E-P added the cleanup label Mar 5, 2025
@P-E-P P-E-P linked an issue Mar 5, 2025 that may be closed by this pull request
Refactor this to use the implicit insert #3395
Open
@P-E-P
Copy link
Member

P-E-P commented Mar 5, 2025

You should probably squash your commits and reword them to include the changelog. Take a look at this section of the wiki page for further informations.

@mostafa630 mostafa630 force-pushed the refactor-implicit-insert-3395 branch from c17c60c to d273963 Compare March 5, 2025 15:38
philberty
philberty requested changes Mar 5, 2025
View reviewed changes
Copy link
Member

@philberty philberty left a comment

Choose a reason for hiding this comment

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

Can you remove the insert_type method entirely from the context object as well?

@mostafa630
Copy link
Author

ok , I will do that

Can you remove the insert_type method entirely from the context object as well?

@mostafa630 mostafa630 closed this Mar 5, 2025
@P-E-P
Copy link
Member

P-E-P commented Mar 6, 2025

ok , I will do that

Can you remove the insert_type method entirely from the context object as well?

You do not need to close the PR each time, you can rebase and rewrite your commits instead.

@mostafa630
Copy link
Author

mostafa630 commented Mar 6, 2025 •
edited
Loading

thank you , I started doing that in the latest pull request , [PR #3489]
i only force the push at each time.

You do not need to close the PR each time, you can rebase and rewrite your commits instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philberty philberty philberty requested changes

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

Successfully merging this pull request may close these issues.

Refactor this to use the implicit insert
3 participants
@mostafa630 @P-E-P @philberty