Skip to content

Make XML format extensible#3222

Closed
ewpatton wants to merge 2 commits into
RaspberryPiFoundation:developfrom
mit-cml:feature/extensible-xml-parser
Closed

Make XML format extensible#3222
ewpatton wants to merge 2 commits into
RaspberryPiFoundation:developfrom
mit-cml:feature/extensible-xml-parser

Conversation

@ewpatton
Copy link
Copy Markdown
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

This is based on work I did at the hackathon in 2018 (yes, yes I know it's late). I'm motivated to add it sooner rather than later though due to some features we want to put into App Inventor that will require new top level elements in the XML.

Proposed Changes

This PR makes it so that one can register tags to be parsed, and can register serialization functions to serialize different parts of the workspace. This change also implements #3025.

Reason for Changes

In App Inventor, we store additional versioning metadata in the XML file that is used to determine when to upgrade from one version of the language to the next. Similarly, we are looking to add new features (such as folders), that might not be part of core Blockly, but still need to be serialized/deserialized preferably without modifying core.

Test Coverage

Ran the npm tests to confirm that nothing broke when parsing/serializing workspaces. Also tested with airstrike in the playground.

Tested on:

  • Desktop Chrome
  • Desktop Firefox
  • Desktop Safari

Documentation

There is now a registerTopLevelTag method that takes:

  • tagName (e.g., 'block')
  • a load function that will be called with the XML element, workspace, and a state
  • a save function that will be called with the workspace, XML root
  • an optional boolean opt_force, that will overwrite any previously registered save/load for the given tagName. This could be used, for example, to overwrite the default handling of <block> from outside of core.

Additional Information

I'm putting this up for PR because I'd like to get some feedback from the team. Mainly:

  1. API design: What are your thoughts on the registerTopLevelTag design?
  2. Code org: I've placed registration of the existing core tags close to domToWorkspace, which is where the original code was located. However, these are executable statements and probably should be moved to the end of the file instead. What's your preference here?
  3. Code org: For workspace comments, since @samelhusseini made them optional, do we want to have registration of the 'comment' tag occur in either Blockly.WorkspaceComment or Blockly.WorkspaceCommentSvg namespaces? Right now there's this check in the parser if the class is defined, but the logic of registering the tag could be moved into the file itself so it simply wouldn't register for parsing unless the supporting requires were also somewhere in the Blockly setup.
  4. Depending on the decision for fix: unit tests were using strict equality testing for numeric results; now check 15 decimals #2, do we want to do the same for other tag types? The downside being that if one day there are multiple parsers (e.g., XML and JSON), the parsing has now been coupled to the type rather than the parser, whereas right now each parser needs to be aware of each type in the system.
  5. onError and onWarning are public as I'm expecting that in App Inventor we'll provide our own implementations, but I can change the scope if desired.

Change-Id: Id88d09120b32a2f7bfa72b89719082f5d4d93b05
Copy link
Copy Markdown
Contributor

@NeilFraser NeilFraser left a comment

Choose a reason for hiding this comment

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

Just some code style comments. No thoughts on the functionality.

Comment thread core/xml.js Outdated
Comment thread core/xml.js Outdated
Comment thread core/xml.js Outdated
Comment thread core/xml.js
if (state.variablesFirst) {
Blockly.Xml.domToVariables(xmlChild, workspace);
} else {
Blockly.Xml.onError('\'variables\' tag must exist once before ' +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

onError throws, so if you negate the if condition then you can drop the else, resulting in smaller, simpler code.

Comment thread core/xml.js
}
state.variablesFirst = false;
} else {
throw TypeError('Shadow block cannot be a top-level block.');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a throw, so negate the if statement, and drop the else.

Change-Id: I83d781ed04e150ec4cc2ee606c876782d608ee0a
@samelhusseini
Copy link
Copy Markdown
Contributor

Should parsing be per workspace? ie: do we see the need for some workspaces to have different parsing logic than others on a page.

@ewpatton
Copy link
Copy Markdown
Contributor Author

@samelhusseini With this solution one could install the appropriate handlers just before parsing a workspace, allowing for per-workspace parsing. I'm not sure that I would want to tie parsing structure to the workspace in case people want to leverage other formats in the future (I've been playing with JSON and protobufs, for example).

@BeksOmega
Copy link
Copy Markdown
Contributor

This will be covered by the new JSON serialization system, so I'm going to go ahead and close this.

@BeksOmega BeksOmega closed this Sep 9, 2021
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.

5 participants