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.

Sign up for GitHub

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

Jump to bottom

#6371 get rid of the OK button #6487

Merged
merged 17 commits into from
Jan 5, 2018
Merged

#6371 get rid of the OK button #6487

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
aa812c9
#6371 remove OK button from template
Dec 11, 2017
0751551
commit contains following changes:
Dec 11, 2017
32bded7
Merge branch 'master' of https://github.com/twosigma/beakerx into luk…
Dec 11, 2017
8c9833f
#6371 remove invalid if statement
Dec 11, 2017
f694f3e
Merge branch 'master' of https://github.com/twosigma/beakerx into luk…
Dec 13, 2017
bcec8f2
#6371 use _.debounce, implement synchronization indicator
Dec 13, 2017
c0e5d56
Merge branch 'master' of https://github.com/twosigma/beakerx into luk…
Dec 14, 2017
56c1cfe
#6371 change sync indicator place in template
Dec 14, 2017
282f3d3
#6371 fix losing focus after sync, and tabing issue
Dec 14, 2017
6e4d2e2
Merge branch 'master' of github.com:twosigma/beakerx into lukasz/6371
piorek Dec 20, 2017
4ff0099
#6371 sync empty values; add only missing inputs when loading settings
piorek Dec 20, 2017
2365c1d
Merge branch 'master' of github.com:twosigma/beakerx into lukasz/6371
piorek Dec 21, 2017
eb15abb
#6371 fix code style (jscs)
piorek Dec 21, 2017
b592d54
#6371 do not send invalid heap size
piorek Dec 21, 2017
cd50fa5
Merge branch 'master' of github.com:twosigma/beakerx into lukasz/6371
piorek Jan 5, 2018
fc3c977
#6371 add additional check when validating heap size
piorek Jan 5, 2018
92974a2
#6371 do not sync data when heap size is invalid
piorek Jan 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 33 additions & 22 deletions beakerx/beakerx/static/tree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,28 +74,21 @@ define(function(require) {
var errors = [];

var val = $('#heap_GB').val().trim();
var parsedVal = parseFloat(val);

if (val !== '') {
if (false === isNaN(parsedVal) && parsedVal > 0) {
if (val % 1 === 0) {
result += '-Xmx' + parsedVal + 'g ';
} else {
result += '-Xmx' + parseInt(parsedVal * 1024) + 'm ';
}
} else {
errors.push('Heap Size must be a positive decimal number.');
try {
if (val !== '') {
var parsedVal = settings.normaliseHeapSize(val);
result += '-Xmx' + parsedVal + ' ';
}
} catch (e) {
errors.push(e.message);
}

var other_property = $('#other_property').find('input');
other_property.each(function() {
$('#other_property').find('input').each(function() {
var value = $(this).val().trim();
result += value + ' ';
});

var java_property = $('#properties_property').find('div');
java_property.each(function() {
$('#properties_property').find('div').each(function() {
var children = $($(this).children());
var value = $(children.get(1)).val().trim();
var name = $(children.get(0)).val().trim();
Expand Down Expand Up @@ -221,6 +214,22 @@ define(function(require) {
}).val(value).keyup(InputChanged);
},

normaliseHeapSize: function(val) {
if (val === '') {
return '';
}

var parsedVal = parseFloat(val);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is broken, please do not change how valid numbers are identified, it's actually quite tricky.
parseFloat("5a") = 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not change the way it was identified

see:

beakerx/beakerx/beakerx/static/tree.js

Lines 54 to 64 in 27dab19

var val = $("#heap_GB").val().trim();
if (val.length > 0) {
if (!isNaN(val)) {
if (val % 1 === 0) {
result += '-Xmx' + val + 'g '
} else {
result += '-Xmx' + parseInt(val * 1024) + 'm '
}
}
}

beakerx/beakerx/beakerx/static/tree.js

Lines 98 to 101 in 27dab19

if (val.length > 0 && isNaN(val)) {
$('#errors').append($('<span>').text("Heap Size must be a decimal number."));
enable_button = false;
}

All i did with it was a little extraction refactor, because i needed that logic in two different places.

I added additional check

  • isFinite(val) will catch "5a"


if (isNaN(parsedVal) || parsedVal <= 0) {
throw new Error('Heap Size must be a positive decimal number.');
}

return (val % 1 === 0) ?
parsedVal + 'g' :
parseInt(parsedVal * 1024) + 'm';
},

load: function(payload) {
if (payload === undefined) {
payload = {
Expand Down Expand Up @@ -375,6 +384,7 @@ define(function(require) {
function _submitOptions() {
var payload = {
version: 2,
heap_GB: '',
jvm_options: {
other: [],
properties: [],
Expand Down Expand Up @@ -403,13 +413,14 @@ define(function(require) {
}

});
var default_property = $('#default_options').find('input');
default_property.each(function() {
var value = $(this).val().trim();
if (value.length > 0) {
payload.jvm_options.heap_GB = value;
}
});

try {
var val = $('#heap_GB').val().trim();
settings.normaliseHeapSize(val);
payload.jvm_options.heap_GB = val;
} catch (e) {
}

settings.setVariables(JSON.stringify({
'beakerx': payload
}));
Expand Down