ODM2 / ODM2PythonAPI

A set of Python functions that provides data read/write access to an ODM2 database by leveraging SQLAlchemy.
http://odm2.github.io/ODM2PythonAPI/
BSD 3-Clause "New" or "Revised" License
4 stars 13 forks source link

Model name fixes #156

Closed darksinge closed 6 years ago

darksinge commented 6 years ago

Updated misspelled column names in odm2api/models.py

lsetiawan commented 6 years ago

There might be a lot of consequences. Looping @emiliom to please test out and review.

emiliom commented 6 years ago

First, a note about personnel changes: @lsetiawan is no longer working on ODM2. @ocefpaf has very limited involvement, and issues involving odm2api details (as opposed to packaging) are outside his domain anyway.

@darksinge I assume you're working with @horsburgh?

From what I can tell, this PR really has two parts:

I think it would be cleaner to accomplish this in two separate steps rather than conflating the two distinct parts. But from what I can tell (by comparing the development and master branches, and looking over the model name fixes), everything seems to be fine in this PR.

@horsburgh can you chime in to confirm you're ultimately behind this?

emiliom commented 6 years ago

One more thing: AppVeyor checks are failing, but I don't remember the status of AppVeyor on this repo. @ocefpaf, do you remember or do you mind checking, if it's not much work?

horsburgh commented 6 years ago

@emiliom - yes, @darksinge is working for me and has made these changes. He's doing some work on the YODA Tools application, which uses ODM2 Python API and noticed these issues that needed to be fixed. If you are OK with us merging these in, we can do it. But, I'm glad you are still watching. We should continue to coordinate our code commits.

ocefpaf commented 6 years ago

@ocefpaf, do you remember or do you mind checking, if it's not much work?

I don't recall the last state of the AppVeyor but the current one is trying to run *nix specific commands.

i-helpers\mysql_setup.sh: line 8: service: command not found
ci-helpers\mysql_setup.sh: line 10: sudo: command not found
cat: /etc/mysql/my.cnf: No such file or directory
ci-helpers\mysql_setup.sh: line 12: sudo: command not found

I recall that @lsetiawan and I got the data base working on some of the repos in the ODM2 or but I guess we never got to ODM2PythonAPI or it was reversed.

Is ODM2PythonAPI expected to run on Windows? If not I recommend to just remove AppVeyor. If it is expect then there are some work to do here and that should not block this PR.

emiliom commented 6 years ago

@horsburgh thanks for chiming in. I'll try to help with odm2api as I can.

@ocefpaf thanks for following up on AppVeyor. We definitely expect odm2api to run on Windows. But we most likely don't have the capacity right now to get AppVeyor to work. So, I guess the best path forward for now is to leave it there and ignore its failures.

I'll go ahead and merge the PR right now.

aufdenkampe commented 5 years ago

@emiliom, "Identifier" was widely mis-spelled in ODM2.0.0 release, but these have been fixed on the develop branch of the ODM2 repo. See https://github.com/ODM2/ODM2/pull/154.

Can you review those changes and align the ODM2 and odm2api repos, making fixes to odm2api?

emiliom commented 5 years ago

@aufdenkampe For the release I'm planning I'm simply going to include the fixes (from this issue) already applied in the development branch, done by someone in Jeff's group mid 2018. I don't have the bandwidth to try to fold in new ones or consider implications of changes to the ODM2 schema (even if they're spelling corrections). The spelling errors corrected in the development branch are errors in odm2api, not in the ODM2 schema.

We can discuss goals for a future odm2api release later, after issuing this one. Bearing in mind of course that none of us have much band width, and we each have our own areas of current ODM2 interest (mine don't stray far from the core). Feel free to open a new odm2api issue on this topic, but please make it fairly narrow.