Skip to content

Conversation

@adutra
Copy link
Contributor

@adutra adutra commented Jan 22, 2025

Note: this assumes #610 is merged as is.

@adutra adutra added the documentation Improvements or additions to documentation, especially web site content label Jan 22, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: whitespace

Comment on lines 46 to 49
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems extremely unintuitive. Is it too late to fix this?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, we shouldn't block the doc change on this behavior -- but if we could fix this behavior that would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this paragraph is written in a very anxiety-provoking tone. It's not that unnatural for developers to first build their code, then start end-to-end tests. Let me rephrase this and I think it could sound a lot more intuitive.

Copy link
Contributor

@dimas-b dimas-b Jan 23, 2025

Choose a reason for hiding this comment

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

I personally find the propose change to be ok, but willing to see an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if that's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

mega nit: minikube

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a spurious change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IntelliJ says it's incorrect:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

IntelliJ says it's less concise in your screenshot which is true, but it's not incorrect and it's not exactly the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, reverting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's locale specific 😅

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

LGTM overall 👍

Copy link
Contributor

@dimas-b dimas-b Jan 23, 2025

Choose a reason for hiding this comment

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

rm -rf ./regtests/output/* simpler (without &&)? Does it work (not tested :) )?

Comment on lines 46 to 49
Copy link
Contributor

@dimas-b dimas-b Jan 23, 2025

Choose a reason for hiding this comment

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

I personally find the propose change to be ok, but willing to see an alternative.

@adutra adutra merged commit 3c3fc95 into apache:main Jan 24, 2025
5 checks passed
@adutra
Copy link
Contributor Author

adutra commented Jan 24, 2025

There has been some confusion around regression tests lately (see #865) and I think this PR would help. So I'm going to merge it now. If there are leftovers, please speak up and I will fix in another PR. Thanks!

@adutra adutra deleted the regtests-docs branch January 24, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation, especially web site content

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants