Skip to content

Conversation

@ahsonkhan
Copy link
Contributor

Bringing back #5390 as part of #5416 (comment)

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

I agree with the recommendation, for you to have the next PR soon that globalizes add_compile_definitions(_azure_TESTING_BUILD).

I think, we should also rename _azure_TESTING_BUILD_AMQP to _azure_TESTING_BUILD (#5403). I would not mind if it was done in this PR.

Also I think that #5414 should be merged the next moment after this PR is merged. For that, I am approving that one as well. You can merge it into main, or you can change the target branch on #5414 PR to merge into your RenameCompileDefinition branch before merging this PR.

You could do the same with #5403 - resurrect the PR, and merge into RenameCompileDefinition instead of main before merging this PR.

@LarryOsterman
Copy link
Member

I agree with the recommendation, for you to have the next PR soon that globalizes add_compile_definitions(_azure_TESTING_BUILD).

I think, we should also rename _azure_TESTING_BUILD_AMQP to _azure_TESTING_BUILD (#5403). I would not mind if it was done in this PR.

Also I think that #5414 should be merged the next moment after this PR is merged. For that, I am approving that one as well. You can merge it into main, or you can change the target branch on #5414 PR to merge into your RenameCompileDefinition branch before merging this PR.

You could do the same with #5403 - resurrect the PR, and merge into RenameCompileDefinition instead of main before merging this PR.

And I don't fully understand why we need to have two separate pull requests which each does exactly the same effect.

I can understand doing this in two PRs if we had a situation where we were actively hampered in getting our job done, but I don't understand why we're making a change which intentionally does the wrong thing under the understanding that some time in the future we'll change it to do the right thing.

@ahsonkhan
Copy link
Contributor Author

I wouldn't say this is a wrong change. It is a change that is certainly in the right direction.

We are naming it to be better, as per the issue and what we agreed on. I want to split up centralizing the definition away from the tactical rename.

It's cleaner to me.

Opinions aside on how you would want to stage the change vs how I did it, are you OK with merging this or do you think it actually makes main worse than it is currently?!

@ahsonkhan ahsonkhan merged commit da15139 into Azure:main Apr 5, 2024
@ahsonkhan ahsonkhan deleted the RenameCompilerDefinition branch April 5, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants