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

Getting rid of Textgrid #467

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Getting rid of Textgrid #467

wants to merge 7 commits into from

Conversation

DennisFriedl
Copy link
Member

Description, Context and related Issue

  • Removed the function "eutil:getDoc" which is not necessary anymore without Textgrid
  • Changed all "eutil:getDoc" calls to standard "doc" calls
  • Removed Textgrid case in an OR statement

#322

How Has This Been Tested?

Tested with the EditionExample.

Types of changes

  • Cleaning up

Overview

@peterstadler
Copy link
Member

My take on that would be a little bit different. I actually like the concept of a dedicated eutil:getDoc function because it provides a canonical way of getting a document (from whatever location). Now we are removing support for TextGrid but maybe we'll add support for GitHub in the future?

Additionally, the standard doc function is not very robust and will fail with an error if it can't parse the resource as XML. Hence such a wrapper function is a very good place for adding proper error handling.

@peterstadler peterstadler added this to the 1.0.0 milestone Nov 8, 2024
@peterstadler peterstadler linked an issue Nov 8, 2024 that may be closed by this pull request
@DennisFriedl
Copy link
Member Author

You are right. Was also thinking of keeping it but thought it would look a little sad with just a return of "doc". But with correct Error handling it gets relevant again.

@DennisFriedl
Copy link
Member Author

I reverted the unwanted changes and got only rid of the textgrid part in the wrapper function. @peterstadler what would be a good way of error handling here? The calls of the function expect a document, so shouldn't "doc()" fail when it can't parse?

@peterstadler
Copy link
Member

It's a little bit beyond the scope of #322 but I couldn't resist to provide my take on a canonical function (including tests) for loading documents within the Edirom.

Copy link
Member

@peterstadler peterstadler left a comment

Choose a reason for hiding this comment

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

tested briefly with the clarinet quintet and all seemed to work

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

Successfully merging this pull request may close these issues.

Remove TextGrid functionality
2 participants