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

Allow parser to handle some missing fields (costs, forces, categories… #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

capajon
Copy link

@capajon capajon commented Oct 24, 2020

…, rules, selections, profiles)

Hi, first thanks for making this project. It's great and very helpful.

I noticed that some (actually most) of the roster files I had previously created in battlescribe error out. I think there are some fields that are not always present (arrays of things, listed above), and the code errors out trying to iterate on them. I'm not an expert on the schema though, but this was output by battlescribe itself.

The simplest solution seemed to be defaulting those to empty arrays in each of the relevant Parser functions. You might know of a better approach though. My apologies too if I'm not following typescript best practices as I am still picking that up.

I also added a test ros file to cover this in your test suite. Not sure if you wanted actual 40k references in your repo though, but I didn't have time to scrub it. It is based on the rosz file I was mostly trying. While I was adding that I refactored the test case a bit to be able to add more test files easily in the future, as no doubt there might be other rosz files that cause a problem, potentially. I figure it will be nice to add odd ball ones as they are discovered.

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #3 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master       #3   +/-   ##
=======================================
  Coverage   99.36%   99.36%           
=======================================
  Files          20       20           
  Lines        1096     1096           
  Branches      176      177    +1     
=======================================
  Hits         1089     1089           
  Misses          3        3           
  Partials        4        4           
Impacted Files Coverage Δ
src/Parser.ts 99.55% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8429ee4...bb16a91. Read the comment docs.

@oscarviq
Copy link

oscarviq commented Jan 21, 2021

@GitHug This probably needs to be pulled?

I just tried to parse a roster for Warhammer 40k and got this error

/Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:128
        bsForces.forEach(function (bsForces) {
^
TypeError: Cannot read property 'forEach' of undefined
    at Parser../node_modules/rosz2js/dist/src/Parser.js.Parser.toForceArray (/Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:128:1)
    at /Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:137:1
    at Array.forEach (<anonymous>)
    at /Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:130:1
    at Array.forEach (<anonymous>)
    at Parser../node_modules/rosz2js/dist/src/Parser.js.Parser.toForceArray (/Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:128:1)
    at Parser.<anonymous> (/Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:74:1)
    at step (/Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:33:1)
    at Object.next (/Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:14:45)
    at fulfilled (/Users/oscarviquez/Code/the-army-summoner/dist/apps/api/webpack:/node_modules/rosz2js/dist/src/Parser.js:5:42)
    at processTicksAndRejections (node:internal/process/task_queues:93:5)

Did a test using capajon's branch and it works. It's a simple fix but it looks like the PR has been sitting there with no merge.

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.

2 participants