-
Notifications
You must be signed in to change notification settings - Fork 35
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
base: dev
Are you sure you want to change the base?
Conversation
Ah ya, I think the use of |
this looks good to me, @mewtaylor !! Nice work, shall I send it into |
@pmalacho-mit yep! it messed up the |
@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. |
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 :). |
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 Let me know if you want to discuss, or if you want me to look at anything! |
@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! |
Redo of #264 - I think the branch naming convention I used was messing up tests!