-
Notifications
You must be signed in to change notification settings - Fork 62
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
Multi document support #568
Multi document support #568
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
a29f4d9
to
8bb0b56
Compare
8bb0b56
to
86f50e3
Compare
iXBRLViewerPlugin/__init__.py
Outdated
@@ -81,6 +81,7 @@ def iXBRLViewerCommandLineOptionExtender(parser, *args, **kwargs): | |||
default=False, | |||
dest="zipViewerOutput", | |||
help="Converts the viewer output into a self contained zip") | |||
parser.add_option("--keepOpen", dest="keepOpen", default=True, action="store_true", help=argparse.SUPPRESS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an existing Arelle option. This line just forces it to True
. Have added a comment in 169b027
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. In that case, when do things get "closed" (e.g., for long-running inside the GUI)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option only affects the command line. I'm not actually sure how to open multiple documents simultaneously in the GUI, but I assume the models are held open until the relevant window is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out that this dubious approach to overriding the default value doesn't work with the most recent Arelle (interferes with new code that differentiates plugin options from base options). Changed to a different, but similarly dubious approach in 74b457d.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the new approach backwards compatible, or does it raise the minimum?
@austinmatherne-wk Do you have any ideas on how we could add a more robust solution to Arelle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be backwards compatible. Newer Arelle relies on counting the number of arguments added by the plugin, and gets confused by re-specifying an existing argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to address a breaking change involving the way CLI options work in Arelle. The current Arelle CLI leverages the deprecated optparse library. Our aim is to transition to argparse. However, this migration is complicated since plugins are directly importing and using optparse to register arguments.
I’m considering the introduction of a new plugin hook named CntlrCmdLine.Arguments
. This would allow plugins to return a list of argument objects (based on a new class in Arelle). These arguments would then be applied by the cntrl to configure optparse, argparse, or any command-line library Arelle adopts in the future. For compatibility, I'm exploring ways to delay making this a breaking change, possibly by passing an empty optparse parser to the existing hook, then iterating over it to discover plugin defined arguments and applying them to the new parser. If a user enables a plugin that still uses the old system, CntlrCmdLine.Options
would log a deprecated plugin hook warning.
I hadn't initially accounted for plugins altering argument options from base Arelle. Based on my plan, the missing keepOpen
argument would break the implementation in this PR with future versions of Arelle. We might be able to allow the new hook to also return a list of argument option overrides, but I'd suggest limiting changes only to the default value. The help text could then automatically be updated by the cntrl, letting users know which plugin modified the default value. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that plugins manipulating command line options in this way is fundamentally the wrong approach, so I think we should probably figure out a better way to solve this rather than compromising your proposed changes for command line options.
We currently also do a similar thing with logToBuffer
but the difference there is that the Arelle core doesn't define that argument, it just reads from it in case a plugin has.
I'm not really sure why logToBuffer
or keepOpen
need to be exposed as end user command line options at all. They seem like internal options on the XBRL processor which plugins rather than end users need to be able to configure, so perhaps we need an alternative way to request these processor options at start up, and then prevent the user from overriding them with command line options?
It's also worth noting that I only used keepOpen
because I couldn't see a good way to pass context between calls to Xbrl.Run
plugin callbacks. If we had that, we could pass an iXBRLViewerBuilder
object between subsequent calls and build the viewer incrementally, rather than doing all reports at the end in the Filing.End
callback.
Likewise for logToBuffer
, we use this to extract validation messages for the report. It would be much nicer if there were an interface for getting validation results directly after loading a model (somewhat related to Arelle/Arelle#760)
QA +10 Testing locally succeeded. |
Reason for change
The viewer needs to support viewing filings comprising multiple separate Inline XBRL Documents or Document Sets.
Description of change
The Python plugin will now convert all loaded reports into the viewer. Each report document will be loaded into a separate tab in the viewer. Note that a source report may be a Inline XBRL Document Set, so each report may contribute multiple tabs to the viewer.
Facts from all reports will be shown in the fact inspector, and search will find facts from all reports.
Steps to Test
Generate a viewer, specifying multiple Inline XBRL source documents. For example, the 5.2.5 series tests from the EFM conformance suite:
Now open the resulting files via a web browser. The result should appear much like an Inline XBRL Document Set with two tabs, but these are actually separate reports, with a separate DTS.
It is also possible to load multiple reports by putting them in a ZIP, or simply listing the filenames separated by
|
.Note that the fact IDs (visible in the browser location bar) are now prefixed with
0-
or1-
for the different documents.There are significant changes to the structure of the JSON data. These are described in
ARCHITECTURE.md
. Some of these changes are designed to cope with multiple target documents, but this is not yet implemented. The viewer should render reports using the old JSON format correctly.review:
@Arelle/arelle
@paulwarren-wk