search

Arelle / ixbrl-viewer

The Arelle iXBRL Viewer allows Inline XBRL (or iXBRL) reports to be viewed interactively in a web browser. The viewer allows users to access the tagged XBRL data embedded in an iXBRL report.
Other
96 stars 59 forks source link

Incremental builder #678

Closed paulwarren-wk closed 2 months ago

paulwarren-wk commented 4 months ago

Reason for change

Fixes #653. Avoids retaining loaded XBRL models indefinitely.

Description of change

Previously the viewer used the keepOpen feature in Arelle to retain XBRL models after the are loaded, so that multiple files can be assembled into a single viewer. This change uses the new pluginData feature to preserve viewer information across models when building multi-document viewers, and then creating the viewer in a separate call after all models are processed.

A slightly awkward feature of this is that we need to retain the XML models until we serialize at the end, but the XML models created by Arelle use custom element classes that are no longer safe to use once the Arelle models have been disposed. I have fixed this by unsetting the custom element class lookup after taking a copy of the model.

Steps to Test

See steps on #653. Can also be confirmed by submitting the same file to the web service multiple times, and confirming that you get exactly the same file back each time.

review: @Arelle/arelle @paulwarren-wk

aviary3-wk commented 4 months ago

Security Insights

No security relevant content was detected by automated scans.

Action Items

paulwarren-wk commented 3 months ago

I'm afraid I'm at a loss with the remaining type linting error

It seems like mypy is failing to find the definition of PluginData. @austinmatherne-wk any ideas?

stevenbronson-wk commented 3 months ago

I'm afraid I'm at a loss with the remaining type linting error

It seems like mypy is failing to find the definition of PluginData. @austinmatherne-wk any ideas?

Arelle is not considered a typed library, so imports from Arelle are treated as Any, and subclassing Any is disallowed. You could use

class IXBRLViewerPluginData(PluginData):  # type: ignore[misc]

to ignore it once, or you could add allow_subclassing_any = true here to allow it everywhere. I might choose the former if this is probably the last Arelle subclass and the latter if not, since it'll fire until Arelle declares types.

paulwarren-wk commented 3 months ago

Arelle is not considered a typed library

Ah, thank you. I'd assumed that as we reference Arelle types elsewhere that we were making use of Arelle type information, e.g.:

    def addFact(self, report: ModelXbrl, f):

But I assume that sub-classing is a special case where the type information is actually needed?

OOI, what is needed for Arelle to be considered a typed library? Is it just missing type annotations, or is there some package configuration needed?

stevenbronson-wk commented 3 months ago

ModelXbrl is also typed as Any there, but Any is compatible with every type, so type errors are rare. One exception is subclassing, which is blocked by disallow_subclassing_any, enabled by mypy strict mode.

We'd need to choose one of the options to support typing. Alternatively, mypy might implement a feature to consider a module as providing types, even if it's not officially supported.

austinmatherne-wk commented 3 months ago

@paulwarren-wk The # type: ignore[misc] option Steven suggested is the best path forward at the moment. We plan to publish the Arelle types, but need to sort out a few internal details first.

paulwarren-wk commented 3 months ago

We'd need to choose one of the options to support typing. Alternatively, mypy might implement a feature to consider a module as providing types, even if it's not officially supported.

Thank you - that's much clearer now.

I've now added the comment to ignore type checks on this line (d5cccaa)

michaelchadfleming commented 2 months ago

@paulwarren-wk @austinmatherne-wk Hi Guys, I hope you had an amazing weekend! :) This is in regards to the comment on https://github.com/Arelle/ixbrl-viewer/issues/653#ref-pullrequest-2305424002. Does this pull request resolve the bug? It appears that there were conflicts with the pull? Thank you for your valuable time, Chad

paulwarren-wk commented 2 months ago

@paulwarren-wk @austinmatherne-wk Hi Guys, I hope you had an amazing weekend! :) This is in regards to the comment on #653 (reference). Does this pull request resolve the bug? It appears that there were conflicts with the pull? Thank you for your valuable time, Chad

Sorry @michaelchadfleming, I dropped the ball on this and didn't realise it was blocked on me. I've just pushed a fix for what I think is the final issue.