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

t: Convert indirect Config syntax in core's tests #21498

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

rwp0
Copy link
Contributor

@rwp0 rwp0 commented Sep 19, 2023

We have a lot of lines throughout the distribution with this particular indirect object notation.

This PR deals with the tests in t/ directory which includes core files only.

Convert:

require Config; import Config;

To:

require Config; Config->import;

Two test files are excluded in this PR:

  • t/op/lc.t: Unicode encoding issues
  • t/re/bigfuzzy.t: Binary data issues

@rwp0 rwp0 changed the title t: Convert indirect syntax in tests t: Convert indirect syntax in core's tests Sep 19, 2023
@rwp0
Copy link
Contributor Author

rwp0 commented Sep 19, 2023

I wonder if using use Config; is a better idea than running two statements in tests (except in eval).

@Leont
Copy link
Contributor

Leont commented Sep 19, 2023

I wonder if using use Config; is a better idea than running two statements in tests.

Making it compile-time would defeat the point of putting it in an eval or running it after @INC has been set up

@rwp0
Copy link
Contributor Author

rwp0 commented Sep 20, 2023

I wonder if using use Config; is a better idea than running two statements in tests.

Making it compile-time would defeat the point of putting it in an eval or running it after @INC has been set up

Thanks. I think it's good to go then!

@nicomen
Copy link
Contributor

nicomen commented Sep 20, 2023

It would be interesting if the various approaches could be made more similar, like should it always be in a BEGIN block, or should it be bundled together with require of test.pl and setting lib somehow.

t/op/lc.t Outdated Show resolved Hide resolved
@rwp0 rwp0 changed the title t: Convert indirect syntax in core's tests t: Convert indirect Config syntax in core's tests Sep 21, 2023
@rwp0
Copy link
Contributor Author

rwp0 commented Sep 24, 2023

It would be interesting if the various approaches could be made more similar, like should it always be in a BEGIN block, or should it be bundled together with require of test.pl and setting lib somehow.

BEGIN

Yes, thanks for the comment, though that's a matter of another PR, I'd like to limit this to reverting the indirect syntax only🙂

@jkeenan
Copy link
Contributor

jkeenan commented Sep 24, 2023

@rwp0, GitHub diff does not display the differences between blead and your branch with respect to t/re/bigfuzzy_not_utf8.t because it says it's a binary file.

Command-line diff also identifies this file as a binary file and says it differs from the version of that file in blead.

[perl2] 2066 $ diff ../perl/t/re/bigfuzzy_not_utf8.t ./t/re/bigfuzzy_not_utf8.t
Binary files ../perl/t/re/bigfuzzy_not_utf8.t and ./t/re/bigfuzzy_not_utf8.t differ

(In the above, blead is checked out into ../perl and your branch into ./perl2 and rebased it on blead.)

But are the differences limited to just your substitution of Config->import for import Config? When I examine the two files with vimdiff, I see that line 37 in blead has been broken into two lines in your branch. According to wc -l, the file is 39 lines long in blead but 40 in your branch. Per superuser, you can also get a diff of the hexdumps of the two versions of the file with something like this:

$ diff <(xxd blead.bigfuzzy_not_utf8.t) <(xxd branch.bigfuzzy_not_utf8.t)

So before merging we have to get bigfuzzy to contain only your intended changes. Can you investigate?

We have a lot of lines throughout
the distribution with this particular
indirect object notation.

This PR deals with the tests in `t/`
directory which includes core files only.

Convert:

```
require Config; import Config;
```

To:

```
require Config; Config->import;
```

`t/op/lc.t`: excluded due to file encoding
(Unicode/Latin-1) issues
@rwp0
Copy link
Contributor Author

rwp0 commented Sep 26, 2023

@rwp0, GitHub diff does not display the differences between blead and your branch with respect to t/re/bigfuzzy_not_utf8.t because it says it's a binary file.

Command-line diff also identifies this file as a binary file and says it differs from the version of that file in blead.

[perl2] 2066 $ diff ../perl/t/re/bigfuzzy_not_utf8.t ./t/re/bigfuzzy_not_utf8.t
Binary files ../perl/t/re/bigfuzzy_not_utf8.t and ./t/re/bigfuzzy_not_utf8.t differ

(In the above, blead is checked out into ../perl and your branch into ./perl2 and rebased it on blead.)

But are the differences limited to just your substitution of Config->import for import Config? When I examine the two files with vimdiff, I see that line 37 in blead has been broken into two lines in your branch. According to wc -l, the file is 39 lines long in blead but 40 in your branch. Per superuser, you can also get a diff of the hexdumps of the two versions of the file with something like this:

$ diff <(xxd blead.bigfuzzy_not_utf8.t) <(xxd branch.bigfuzzy_not_utf8.t)

So before merging we have to get bigfuzzy to contain only your intended changes. Can you investigate?

That file indeed contains binary data passed as a function argument.

This fact unfortunately seem to mess with my IDE and tools.

I excluded the t/re/bigfuzzy.t file from my changes as well, and noted that in the commit message.

I'm noting to work on these two excluded files later in another PR, perhaps individually.

Thanks for the review

@rwp0
Copy link
Contributor Author

rwp0 commented Nov 3, 2023

I think this is good to go too now.

Encode fixed its tests as well in dankogai/p5-encode#176 upstream.

@Leont Leont merged commit 802ca53 into Perl:blead Nov 3, 2023
28 checks passed
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.

5 participants