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

PM4 Pull request #48

Merged
merged 103 commits into from
May 24, 2018
Merged

PM4 Pull request #48

merged 103 commits into from
May 24, 2018

Conversation

kostapsimoulis
Copy link
Contributor

This is the final pull request for PM4. A couple of things might be added later:

  • Support/Test other versions of Kinect. Right now we have only tested 1414
  • Finalize and test docker configuration

smokhov and others added 30 commits March 6, 2018 18:20
…ng enable and disable mix flag paths to api.
@smokhov smokhov added Java web services OpenISS as a service labels Apr 27, 2018
@smokhov
Copy link
Member

smokhov commented Apr 28, 2018

  • There are conflicts in here to be resolved... did you sync the branches and my master, @kostapsimoulis ?

Copy link
Member

@smokhov smokhov left a comment

Choose a reason for hiding this comment

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

Why bin/libfreenect2/ was checked in with all the unneeded READMEs?

@@ -0,0 +1,49 @@
Source code: https://github.com/OpenKinect/libfreenect2
API reference: http://openkinect.github.io/libfreenect2

Copy link
Member

Choose a reason for hiding this comment

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

  • I don't believe this file is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following files/directories were accidentally added by Eclipse users. Then someone noticed that and added them to .gitignore but they didn't clean up before. I just cleaned up the following files/folders and it should solve this issue:

bin
.classpath
.settings
.project

@@ -1,12 +1,12 @@
#version 330 core

Copy link
Member

Choose a reason for hiding this comment

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

  • why this required a change in the context of your project?

import java.nio.ShortBuffer;
import java.nio.FloatBuffer;

import java.nio.*;
Copy link
Member

Choose a reason for hiding this comment

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

It is proper to expand java.nio.* to what modules are actually used instead of bulk import of everything.

import javax.xml.ws.Service;

@WebServlet(name = "OpenISSSOAPClient", urlPatterns = {"/OpenISSSOAPClient"})
public class OpenISSSOAPClient extends HttpServlet {
Copy link
Member

Choose a reason for hiding this comment

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

What happened to OpenISSSOAPClient? Why removed?

@smokhov
Copy link
Member

smokhov commented Apr 28, 2018

  • Why the entire bin directory was added with all the compiled stuff? It tremendously pollutes this PR.

@smokhov
Copy link
Member

smokhov commented Apr 28, 2018

  • Tons of binary files added, executables, libraries, all over the place under src/ as well

@smokhov
Copy link
Member

smokhov commented Apr 28, 2018

  • Unrelated changes to src/api/c/examples/ogl/*... why?

@smokhov
Copy link
Member

smokhov commented Apr 28, 2018

  • Permissions on files 0 src/scripts/build.sh 100755 → 100644
  • src/scripts/dependencies/el6.sh 100755 → 100644
  • tools/ci/docker_run_tests.sh 100755 → 100644
  • tools/ci/docker_setup_tests.sh 100755 → 100644

Changed? Why? They should remain executable.

@smokhov
Copy link
Member

smokhov commented Apr 28, 2018

  • please confirm SOAP client and server are still intact and fully functional @kostapsimoulis

Copy link
Member

@smokhov smokhov left a comment

Choose a reason for hiding this comment

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

The commit 4027819 does the exact opposite of what it says. It adds all of bin/.

@kostapsimoulis kostapsimoulis changed the base branch from team10 to master April 29, 2018 21:25
@kostapsimoulis kostapsimoulis changed the base branch from master to team10 April 29, 2018 21:44
@kostapsimoulis
Copy link
Contributor Author

kostapsimoulis commented Apr 29, 2018

  1. Conflicts have been fixed by syncing with upstream as requested.

  2. bin directory, unrelated changes and bad permissions were all part of an accidental commit that has been reverted. The accidental commit was done in order to clean up some files that were added by Eclipse and they were pushed by accident as well. Those files were later added to .gitignore but that was done after being pushed and it created a bit of a confusion. The request has been cleaned up now from 311 files down to 79 files changed. Accidental commit: kpsimoulis@4027819

  3. src/api/java/openiss/ws/soap/client/OpenISSSOAPClient.java was never implemented properly, it never worked and it was creating issues to keep it there. It had to be removed in order to clean up everything before merging. The SOAP Client has been written successfully and tested in Node.js in the javascript directory

  4. Soap client and server have been tested several times and they are still intact. Please refer to test cases for soap in our documentation for more details about testing.

const express = require('express');
let serviceLibrary = process.env.NODE_WEB_SERVICE || 'rest.js';
const service = require('./lib/'+serviceLibrary);
var morgan = require('morgan');
Copy link
Contributor

Choose a reason for hiding this comment

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

should avoid using var keyword, use let instead.

Copy link
Member

Choose a reason for hiding this comment

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

Extracted into issue #53

Copy link
Contributor

@jashanj0tsingh jashanj0tsingh left a comment

Choose a reason for hiding this comment

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

A very few hard coded paths for instance Class- Test2 variables inputFile/outputFile and PORT variable in itself can be a part of global config params and can be appended to the url constants. Otherwise, it is okay to merge this pr.

@smokhov
Copy link
Member

smokhov commented May 24, 2018

Put @jashanj0tsingh's comment to review in issue #54

@smokhov smokhov merged commit 1d0c066 into OpenISS:team10 May 24, 2018
@smokhov smokhov mentioned this pull request May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Java API
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet