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

Feature: table labels #265

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Feature: table labels #265

wants to merge 8 commits into from

Conversation

mewtaylor
Copy link
Collaborator

Redo of #264 - I think the branch naming convention I used was messing up tests!

@pmalacho-mit
Copy link
Collaborator

Redo of #264 - I think the branch naming convention I used was messing up tests!

Ah ya, I think the use of / confuses things (is that what you were thinking?).

@pmalacho-mit
Copy link
Collaborator

this looks good to me, @mewtaylor !! Nice work, shall I send it into dev, or do you have anything else you want to do with it?

@mewtaylor
Copy link
Collaborator Author

@pmalacho-mit yep! it messed up the rsync

@mewtaylor
Copy link
Collaborator Author

@pmalacho-mit hmm before you send it to dev, lemme check how loading the template for DoAI works, and if it breaks because it has existing tables but no table names. I'd love to have it cover old templates without needing to redo them again this close to DoAI day.

@pmalacho-mit
Copy link
Collaborator

@pmalacho-mit hmm before you send it to dev, lemme check how loading the template for DoAI works, and if it breaks because it has existing tables but no table names. I'd love to have it cover old templates without needing to redo them again this close to DoAI day.

Sounds good!! Totally makes sense. Let me know if you need any help.

@mewtaylor
Copy link
Collaborator Author

ok @pmalacho-mit confirmed that this does in fact break the existing templates for DPW and DoAI. I'm gonna think a bit about if it's worth it to adjust the extension to gracefully handle old projects or not, given the newness of the extension to begin with, and get back to ya.

In the meantime, let's hold off on merging this :).

@pmalacho-mit
Copy link
Collaborator

Sounds good @mewtaylor ! Were you able to diagnose the source of the errors on load? I wonder if there's a little bit of 'cleaning' you can do in the load handler to adrress the issues (say, if not having row / column names saved is causing the issues).

Let me know if you want to discuss, or if you want me to look at anything!

@mewtaylor
Copy link
Collaborator Author

@pmalacho-mit exactly, yup! And it isn't too cumbersome from a coding standpoint to not do it, so I added it here and that works :). I think with this fix we should be ok to merge, but take a look if you have a minute and lemme know what you think!

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

Successfully merging this pull request may close these issues.

None yet

2 participants