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

fix(xml) fix xml.parse() with filename #431

Merged
merged 2 commits into from
Jul 21, 2022
Merged

fix(xml) fix xml.parse() with filename #431

merged 2 commits into from
Jul 21, 2022

Conversation

cwalther
Copy link
Contributor

Penlight 1.12.0 has a bug where giving a file name to xml.parse() (with second argument true) will return nonsense (the file name again). It was broken in b749d88 due to a mistake in local variable scoping.

The attached commits add a test and fix the bug.

alerque
alerque approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I suppose fixing this won't hurt anybody and when we do remove it will just make a better starting point for somebody to peal it off as a separate module, but you have seen #409 right?

@cwalther
Copy link
Contributor Author

Yes, I’m aware of the deprecation. But as far as I understand, removing the deprecated functionality will only happen on the next major version, and I have no idea how far out that is. The fix was simple enough not to leave it broken until then. Also, fixing it was easier than switching our code that uses this over to a different XML library. I prefer to put that off until I have to.

Tieske
Tieske requested changes Jun 13, 2022
View reviewed changes
Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

thx for the fix. Would you mind adding a changelog entry?

@cwalther
Copy link
Contributor Author

I can do that if you tell me what version it should go under (1.12.1 or 1.13.0) and in what order (added at the top or at the bottom or sorted by category).

@alerque
Copy link
Member

alerque commented Jun 16, 2022

Since we already have a few feat: commits since 0.12.0 the next release should be a minor bump to 0.13.0 and not a patch release. I think we've usually top-posted changelog entries, but since you are starting a new section for this single change that shouldn't matter.

@cwalther
fix(xml) fix xml.parse() with filename
a9a5a56 
xml.parse(filename, true, true) would return filename instead of reading
and parsing the file of that name. Broken by b749d88 due to a mistake in
local variable scoping.
@cwalther cwalther requested a review from Tieske June 16, 2022 15:45
Tieske
Tieske approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

just got CI fixed for Windows. Thx @cwalther !

@Tieske Tieske changed the title Fix xml.parse() with filename fix(xml) fix xml.parse() with filename Jul 21, 2022
@Tieske Tieske merged commit 30db6eb into lunarmodules:master Jul 21, 2022
@cwalther
Copy link
Contributor Author

Thanks!

@cwalther cwalther deleted the xmlparse branch July 21, 2022 17:31
@bungle bungle mentioned this pull request Aug 8, 2022
Merged
bungle added a commit to Kong/kong that referenced this pull request Aug 8, 2022
@bungle
chore(deps) bump penlight from 1.12.0 to 1.13.1
c7ce06a 
### Summary

#### 1.13.1 (2022-Jul-22)
 - fix: `warn` unquoted argument
   [#439](lunarmodules/Penlight#439)

#### 1.13.0 (2022-Jul-22)
 - fix: `xml.parse` returned nonsense when given a file name
   [#431](lunarmodules/Penlight#431)
 - feat: `app.require_here` now follows symlink'd main modules to their directory
   [#423](lunarmodules/Penlight#423)
 - fix: `pretty.write` invalid order function for sorting
   [#430](lunarmodules/Penlight#430)
 - fix: `compat.warn` raised write guard warning in OpenResty
   [#414](lunarmodules/Penlight#414)
 - feat: `utils.enum` now accepts hash tables, to enable better error handling
   [#413](lunarmodules/Penlight#413)
 - feat: `utils.kpairs` new iterator over all non-integer keys
   [#413](lunarmodules/Penlight#413)
 - fix: `warn` use rawget to not trigger strict-checkers
   [#437](lunarmodules/Penlight#437)
 - fix: `lapp` provides the file name when using the default argument
   [#427](lunarmodules/Penlight#427)
 - fix: `lapp` positional arguments now allow digits after the first character
   [#428](lunarmodules/Penlight#428)
 - fix: `path.isdir` windows root directories (including drive letter) were not considered valid
   [#436](lunarmodules/Penlight#436)
bungle added a commit to Kong/kong that referenced this pull request Aug 8, 2022
@bungle
chore(deps) bump penlight from 1.12.0 to 1.13.1 (#9206)
6c9a266 
### Summary

#### 1.13.1 (2022-Jul-22)
 - fix: `warn` unquoted argument
   [#439](lunarmodules/Penlight#439)

#### 1.13.0 (2022-Jul-22)
 - fix: `xml.parse` returned nonsense when given a file name
   [#431](lunarmodules/Penlight#431)
 - feat: `app.require_here` now follows symlink'd main modules to their directory
   [#423](lunarmodules/Penlight#423)
 - fix: `pretty.write` invalid order function for sorting
   [#430](lunarmodules/Penlight#430)
 - fix: `compat.warn` raised write guard warning in OpenResty
   [#414](lunarmodules/Penlight#414)
 - feat: `utils.enum` now accepts hash tables, to enable better error handling
   [#413](lunarmodules/Penlight#413)
 - feat: `utils.kpairs` new iterator over all non-integer keys
   [#413](lunarmodules/Penlight#413)
 - fix: `warn` use rawget to not trigger strict-checkers
   [#437](lunarmodules/Penlight#437)
 - fix: `lapp` provides the file name when using the default argument
   [#427](lunarmodules/Penlight#427)
 - fix: `lapp` positional arguments now allow digits after the first character
   [#428](lunarmodules/Penlight#428)
 - fix: `path.isdir` windows root directories (including drive letter) were not considered valid
   [#436](lunarmodules/Penlight#436)
StarlightIbuki pushed a commit to Kong/kong that referenced this pull request Aug 9, 2022
@bungle @StarlightIbuki
chore(deps) bump penlight from 1.12.0 to 1.13.1 (#9206)
a25a8a8 
### Summary

#### 1.13.1 (2022-Jul-22)
 - fix: `warn` unquoted argument
   [#439](lunarmodules/Penlight#439)

#### 1.13.0 (2022-Jul-22)
 - fix: `xml.parse` returned nonsense when given a file name
   [#431](lunarmodules/Penlight#431)
 - feat: `app.require_here` now follows symlink'd main modules to their directory
   [#423](lunarmodules/Penlight#423)
 - fix: `pretty.write` invalid order function for sorting
   [#430](lunarmodules/Penlight#430)
 - fix: `compat.warn` raised write guard warning in OpenResty
   [#414](lunarmodules/Penlight#414)
 - feat: `utils.enum` now accepts hash tables, to enable better error handling
   [#413](lunarmodules/Penlight#413)
 - feat: `utils.kpairs` new iterator over all non-integer keys
   [#413](lunarmodules/Penlight#413)
 - fix: `warn` use rawget to not trigger strict-checkers
   [#437](lunarmodules/Penlight#437)
 - fix: `lapp` provides the file name when using the default argument
   [#427](lunarmodules/Penlight#427)
 - fix: `lapp` positional arguments now allow digits after the first character
   [#428](lunarmodules/Penlight#428)
 - fix: `path.isdir` windows root directories (including drive letter) were not considered valid
   [#436](lunarmodules/Penlight#436)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tieske Tieske Tieske approved these changes

@alerque alerque alerque approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cwalther @alerque @Tieske