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

Analysis waiting page handshake #182

Closed
wants to merge 4 commits into from

Conversation

squirrelo
Copy link
Contributor

Removes string parsing element of JSON handshake.

ElDeveloper and others added 3 commits June 23, 2014 22:02
Note that this commit ports over the contents of the previously existing
repository.

Fixes qiita-spots#59
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling dcfb331 on squirrelo:handshake into 59e835c on biocore:master.

@adamrp
Copy link
Contributor

adamrp commented Jul 2, 2014

The code looks fine, but what is test.txt? It looks like it's in the root qiita directory. Did you mean to include it, or was this the result of another git add .?

@squirrelo
Copy link
Contributor Author

I honestly have no idea where that came from. And no more git add . after the last fiasco....

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling da4248d on squirrelo:handshake into 59e835c on biocore:master.

@josenavas
Copy link
Contributor

Did you pull form upstream? This file was added by mistake in one of my PR but I already solved in the past.

Just to make sure, can you also run the tests and make sure that this files does not get generated?

@squirrelo
Copy link
Contributor Author

Yeah I pulled from upstream. Doesn't seem like it's coming from tests.

@squirrelo
Copy link
Contributor Author

Also this should be ready to merge if there are no more comments.

@adamrp
Copy link
Contributor

adamrp commented Jul 2, 2014

I personally don't like having the javascript visible in the HTML template like this. What do other devs think?

@squirrelo
Copy link
Contributor Author

It's going to be visible whether it's in a separate file or within the page itself. There's no security to be had by moving it.

@adamrp
Copy link
Contributor

adamrp commented Jul 2, 2014

Was not a security concern, just a code clutter concern

@adamrp
Copy link
Contributor

adamrp commented Jul 2, 2014

Also, with respect to the test.txt file, you should be able to run git clean -xdf in your git folder and it should go away if it's not part of the repository (just be very careful, because this will remove anything that's not part of the repo!)

@squirrelo
Copy link
Contributor Author

Ah. My logic is that the javascript in the head of templates is always page-specific. I figured that would be the best way to keep things organized and together.

@ElDeveloper
Copy link
Member

Closing this as the git history seems to be corrupted, @squirrelo agreed to open a new PR for this.

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.

None yet

5 participants