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

Model name fixes #156

Merged
merged 4 commits into from
Jul 24, 2018
Merged

Model name fixes #156

merged 4 commits into from
Jul 24, 2018

Conversation

darksinge
Copy link

Updated misspelled column names in odm2api/models.py

@darksinge darksinge requested review from ocefpaf and lsetiawan July 19, 2018 23:08
@lsetiawan
Copy link
Member

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

@lsetiawan lsetiawan requested a review from emiliom July 20, 2018 15:44
@emiliom
Copy link
Member

emiliom commented Jul 24, 2018

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:

  • Merging into the development branch changes already applied to master and pushed out as release v0.7.1. In other words, making the development branch in sync with the latest release. This is made up of the first 3 components (2 PR's and a commit).
  • Correcting model name spelling mistakes in models.py. That's this commit

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
Copy link
Member

emiliom commented Jul 24, 2018

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
Copy link
Member

@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
Copy link
Member

ocefpaf commented Jul 24, 2018

@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
Copy link
Member

emiliom commented Jul 24, 2018

@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.

@emiliom emiliom merged commit de5a61c into development Jul 24, 2018
@ocefpaf ocefpaf deleted the model-name-fixes branch July 24, 2018 18:51
@emiliom emiliom added this to the v0.7.2 release milestone Apr 8, 2019
@emiliom emiliom mentioned this pull request Apr 8, 2019
@aufdenkampe
Copy link
Member

aufdenkampe commented Apr 23, 2019

@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 ODM2/ODM2#154.

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

@emiliom
Copy link
Member

emiliom commented Apr 23, 2019

@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.

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.

6 participants