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

Show concept taxonomy #305

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

paulwarren-wk
Copy link
Contributor

@paulwarren-wk paulwarren-wk commented May 22, 2022

This PR adds an additional field to the fact inspector that gives a human-readable label for the taxonomy that the concept is drawn from.

The primary motivation for this is so that the version of the taxonomy is visible. The inspector already shows the prefix used for the concept's namespace, but this is typically a non-version-specific string such as us-gaap or ifrs-full.

The taxonomy names are stored in a translation file.

image

Taxonomy names are also in the highlight key:

image

@aviary-wf
Copy link

Security Insights

(1) Vulnerable direct dependencies were detected
  • 1 vulns in numpy < 1.21 via requirements.txt
  • Action Items


    Questions or Comments? Reach out on Slack: #support-infosec.

    @paulwarren-wk paulwarren-wk marked this pull request as ready for review May 22, 2022 22:08
    @paulwarren-wk paulwarren-wk requested a review from a team as a code owner May 22, 2022 22:08
    const nsURI = fact.concept().qname().namespace;
    $('tr.taxonomy td', factHTML).text(this._report.taxonomyNameForURI(nsURI, nsURI));
    }
    else if (fact instanceof Footnote) {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Pre-existing, but would it be better to use item naming (throughout) rather than fact? _itemSummaryHTML, ci (current item?), etc.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Have split into separate methods so that we can have a descriptive variable name in both branches, and renamed to item in the common code.

    // taxonomies file.
    // Returns def if none available.
    iXBRLReport.prototype.taxonomyNameForURI = function(nsURI, def) {
    const nsURIEscaped = nsURI.replaceAll(/[.:]/g,'_');
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Perhaps it's worth a small comment mentioning nsSeparator and keySeparator to make it clearer why we're doing these replacements? I'm not so familiar with i18next, so it took me a bit to dig up why they were special.

    iXBRLViewerPlugin/viewer/src/js/inspector.js Outdated Show resolved Hide resolved
    iXBRLViewerPlugin/viewer/src/js/inspector.js Outdated Show resolved Hide resolved
    @@ -0,0 +1,15 @@
    {
    "http_//fasb_org/us-gaap/2022": "US GAAP 2022",
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I somewhat worry this file will become difficult to maintain, either we don't remember to update it or we end up adding many things to it. I can already guess we're going to get support requests from customers (or our own professional services team) asking about inconsistencies. I wonder if it would be nicer to allow the names to be passed on the command-line when generating a viewer so that the ixbrl-viewer.js doesn't need to be rebuilt to add new names? Long-term, it would be nice if we could figure out how taxonomies could just provide these terse labels themselves, perhaps with a generic linkbase and a namespace-as-resource?

    @derekgengenbacher-wf Thoughts on how we might manage this?

    Copy link
    Contributor

    @sagesmith-wf sagesmith-wf May 23, 2022

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Not Derek :) but I like the idea of a command-line option, it seems completely reasonable. The generic linkbase idea would be neat, but only if we could get regulators to start providing it somehow

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Another possible place to put it would be in the Taxonomy Package metadata. That already provides the option of multilingual name and description. It doesn't provide a short name option at the moment, but the schema is intentionally extensible, so it's something we could suggest.

    It does already provide a separate version field.

    The problem is how to then link it with specific namespaces, as a Taxonomy Package may define multiple concept-bearing namespaces.

    This is probably something to raise with the Spec WG.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    To address the original point, I'm happy to move this to be a viewer-build-time input, rather than hard-coded into the JS, but I think it makes sense to provide some defaults.

    I wonder if this is something we should actually address in Arelle? For US taxonomies, Arelle already has the edgartaxonomies.xml file which looks to provide exactly what we need, and is already being maintained in the Arelle codebase.

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Another possible place to put it would be in the Taxonomy Package metadata.

    I think we want it discoverable from the XBRL docs, and I wouldn't expect that metadata to be referenced by the DTS (sounds backwards).

    I'm happy to move this to be a viewer-build-time input, rather than hard-coded into the JS, but I think it makes sense to provide some defaults.

    That seems reasonable to me.

    For US taxonomies, Arelle already has the edgartaxonomies.xml file

    If adding ESEF files and exposing that with some kind of Python API seems reasonable to Herm, et al, then that seems somewhat more palatable than trying to maintain a registry in this repo.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I'll discuss with Herm. IFRS is already in that Edgar list, which the main thing that we care about for ESEF.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Raised https://gitlab.xbrl.org/base-spec/base-spec/-/issues/292 to discuss a standard XBRL mechanism for this.

    Copy link
    Contributor

    @sagesmith-wf sagesmith-wf left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @paulwarren-wk , it seems like all comments have been addressed and this seems like a good improvement. Do you have any hesitations with resolving merge conflicts and opening not as a draft?

    @brettkail-wk
    Copy link
    Contributor

    @sagesmith-wf It's not clear to me that this thread was actually addressed (i.e., I'm not sure how our systems or other users would be able to plug in their own custom taxonomy mappings for whatever reason).

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

    Successfully merging this pull request may close these issues.

    5 participants