Skip to content
This repository has been archived by the owner on Dec 18, 2020. It is now read-only.

Patch ini parsing #18

Closed
xan105 opened this issue Oct 5, 2020 · 6 comments
Closed

Patch ini parsing #18

xan105 opened this issue Oct 5, 2020 · 6 comments
Assignees

Comments

@xan105
Copy link

xan105 commented Oct 5, 2020

Reminder to use the module ini with this patch to avoid a problematic case where if a achievement's apiname contains a . it would interfere in the ini to js object conversion of said module as a . is used to separate object properties.

Recommended to use the module patch-package as dev dep. to automatize patching when using npm i

"dependencies": {
    "ini": "^1.3.5"
},
 "devDependencies": {
    "patch-package": "^6.2.1"
}

Fix bug where a "." in ini file section for ach name would fail because the ini parser would split it into nested object

cf: achievement-watcher/achievement-watcher@1f26aa2a0e42632f6e946cedc780dfbe77c359c8

@msolefonte msolefonte self-assigned this Oct 5, 2020
@msolefonte msolefonte changed the title ini parsing Patch ini parsing Oct 5, 2020
@msolefonte
Copy link
Owner

I am actually not comfortable with this. Nor patching packages nor using packages that need to be patched. How is it possible that a package like ini, by npm, holds this kind of problem? Aren't there alternatives?

@xan105
Copy link
Author

xan105 commented Oct 7, 2020

This is the discussion of the problem : npm/ini#60
There are more than one pr to solve it. But they aren't merged.
Not the first time I had to use patch-package for this kind of case where I need a pr merged, etc

I do not have experience with other npm package for ini parsing (if there is any).

@msolefonte
Copy link
Owner

Added at #39. I tried multiple libraries and js-ini is the lighter one that works.

@xan105
Copy link
Author

xan105 commented Oct 16, 2020

I confirm that js-ini doesn't go boom with a dot.

[achievement.goes.boom01]
timestamp = 0
"use strict"

const oldini = require("ini");
const ini = require("js-ini");
const fs = require('fs');

const raw = fs.readFileSync('test.ini', 'utf8');

console.log(ini.parse(raw));
console.log(oldini.parse(raw));
{
  'achievement.goes.boom01': { timestamp: 0 }
}
{
  achievement: { goes: { boom01: [Object] } }
}

@msolefonte
Copy link
Owner

I actually added these tests:

Pull
...
√ All active achievement names are in the schemas
√ All active achievement unlock times are valid
√ All active achievement progress values are valid
...

This issue is now automatically tested with the first test. If the achievement name is not in the schema, it throws a very specific error notifying the conflicting platform, source, appid and achievement name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants