search

bseddon / XBRL

Classes that implement the XBRL 2.1, XDT, Formulas, Generics, XFI and other XBRL specifications
GNU General Public License v3.0
41 stars 16 forks source link

Several fixes (to be able to handle the Belgian tax taxonomy) #5

Closed tim-vandecasteele closed 5 years ago

tim-vandecasteele commented 5 years ago

Hi,

I played around with the code to check if I could get it running doing assertions based on the Belgian tax taxonomy (webpage only available in Dutch and French: https://financien.belgium.be/nl/E-services/biztax/technische-documentatie#q2).

In the end I got it working, but I needed to do several fixes. I believe they are bugfixes, but not 100% sure of all.

I created a small test project to get things up and running, with the source code of the necessary libraries installed via composer. You can find the changes here: https://github.com/tim-vandecasteele/xbrl-experiment/commits/tim-fixes-for-be-tax . They are split per commit with an explanation of what and how.

I didn't transfer these PRs to the corresponding repositories (3 for xpath2 and 3 for xbrl) , as I'm not clear on what the policy is, specifically around tests. Let me know if this makes sense and what the process is to get these landed.

bseddon commented 5 years ago

A big thank you for letting me know about the changes you have made. What a great email to receive on a Monday morning. I hope you managed to have some personal time this weekend because I'm sure it took you a while to fix these issues.

I've reviewed your changes and they all seem like bugs. A challenge has been that the conformance suites are missing many possible scenarios. Even though the suites include many thousands of tests, the flexibility of XBRL XML flexibility make it easy to overlook possible scenarios and you have identified several. But it's great to know that with a modest number of changes the BE taxonomies can be supported.

Have you had time to run the modified code through the respective conformance suites to make sure the changes do not affect existing tests? I'll assume not so press ahead with making your changes to my copy and run the tests. If you have allready run the tests it will be great to learn of the results.

bseddon commented 5 years ago

Did you test against a specific year version of the BE taxonomy? Of course the code should work for all years but in the first instance it will be ideal to run against the same version you have used so I'm not running into additional issues due to differences across the versions,.

tim-vandecasteele commented 5 years ago

@bseddon hi, I ran it against the latest version. See archive attached.

Sadly I cannot give you a file to test against, as it contains confidential data. Archive.zip

I removed one assertion file because it contains a specific xfi formula that's not supported (still looking at that) and I added a few general xbrl files (still need to look why the general files are not taken from the repo).

Do you mean the examples.php for the conformance suite? If so, I'll run it and let you know the results, if not, let me know how I can run it.

bseddon commented 5 years ago

@tim-vandecasteele Thanks for the response and the archive. I can see the change you have made to be-tax-f-itifa-5327-2019-04-30-assertion.xml.

You have answered my quesiton about testing. There is a separate project in a repository in my account that aims to run the various XBRL supplied conformance suites against a version of the code.

Now I know this is not something you have done I will attempt to run these tests against the changes you suggest to make sure they do not break something else. I'll let you know though probably tomorrow. If the tests are successful, I will update the respective repositories. If they are not successful, I'll investigate further,

I'll also look at the 'missing' XFI. Maybe there is a custom function that has been defined that is not being recognised.

tim-vandecasteele commented 5 years ago

@bseddon thanks for looking into the commits. Do you want me to make pull requests out of this, or will you just take the changes along?

bseddon commented 5 years ago

The XFI error is occurring because the taxonomy uses an XFI function (fact-explicit-scenario-dimension-value) that is not part of the XBRL function registry. The full list of supported functions is documented in the XFI registry on the XBRL web site. The XFI function does exist and has a sequence number of 90301 but never made it to the 'recommended' status so is not supported by the PHP formula processor.

bseddon commented 5 years ago

We posted at the same time.

I have updated my copy and will upload it to my repository once I've completed the review of the issues and fixes you report. I'm still looking at the 'duplicate' formula issue. The formula IS effectively duplicated in the DTS so before relaxing the current prohibition I have reached out to one of the original authors of the formula specification to try to find out what it is envisaged really should happen in this scenario. Your fix is probably correct and the current restriction is probably wrong but if possible it will be ideal to get another, hopefully authoritative perspective first.

bseddon commented 5 years ago

As far as the XFI function goes, I have added a placeholder function to XBRL-functions.php so if you are able to find a specification for this formula you will have a place to create an implementation. In the meantime it will throw an exception.

tim-vandecasteele commented 5 years ago

@bseddon awesome, thanks so much for the feedback!

I'm still playing around with it, so I'll let you know when I encounter other problems.

bseddon commented 5 years ago

Rather than have the non-recommended function fail, I've redirected it to the supported function fact-explicit-dimension-value. I suspect the difference is that the supported function will access a dimension with a segment OCC first and a scenario OCC second. Redirecting the way I have should work in most cases but could lead to an incorrect computation if the formula is expecting an error when a dimension uses a segment OCC but the function returns a value when perhaps it should not. However, the taxonomy should not be using the non-supported XFI function in the first place.

bseddon commented 5 years ago

This morning my implementations of five of your six suggestions plus change for the XFI function have been pushed to the dev branch of XBRL and XPath2 repositories. You can search for your Github name to locate the changes (except the XFI one) where it will appear in the comments with a link to the issue description in your commits. Still looking at the sixth issue.

tim-vandecasteele commented 5 years ago

@bseddon great! Thank you very much.