labofsahil/github-event-dataset-2012 huggingface dataset:
Could we perhaps use a map instead of array for constant lookup time and more flexibility in regards to storing the boolean flag?  It should not be needed to pass in the persistent flag when removing a dialog (should only need to provide the dialog in question).  Beyond that, using the combination of dialog and flag could cause problems if the persistent flag is changed to be mutable.
It's also worth asking - is there any view which does NOT meet the criteria for optimization currently, with these parameters? Is it even a necessary check anymore? It doesn't seem like it.
NEVERMIND... I'm an idiot.
I will move the declaration above the error check and keep the assignment afterwards.
Hmm, ok. That makes sense. Also, since changeActiveTab is used in a few places, creating a new method would reduce the risk of regressions.
Should call mViewProxy.isLocaleProperty() and mViewProxy.updateLocaleProperty().
Why not up with the other vars?
split string path segments into separate path.join() args
Seems like this TODO can be removed if the comments are addressed; there might need to be a more specific one for the `layoutSubviews` line alone, to indicate we need to evaluate it (if any TODO at all).
Debug leftover ?
Residue of how it was done in the previous tests.  Will work this in.
A stronger condition is to throw when `typeof value !== 'number'`. If an object was stored as the value, `_retrieve` auto-parses; while the object is not a string, it can't be incremented. 

You can stress test the function in your specs by trying to incr values that are of non-numerical types (arrays, objects, strings) since they are possible storage types.
It's a low priority enhancement. No need to worry about that yet.
Would this work when typing at the end of the document (`command.offset` is equal to the document length)?
Может лучше проверять не !...empty?, а сразу .present?
use foreach and precompute `(bits+31)/32` outside the loop
Well what happened to the "thou shall keep the existing style in modules one is changing"? One simple matter is that the variable-indent style requires editor support.

>I'm certainly not going to fault you for preferring a different indentation style, but I'm also not going to agree that we should try and use the indentation style that you're suggesting everywhere.

Jonathan, sorry, this is just inappropriate. The matter of faulting one for one's style (particularly if just a mainstream one) is not even in the question, so it's odd to even mention. And this is not a suggestion - it's the style that has been used consistently in this very 9KLOC module.
Oh come on... you can't be serious :)
This code was changed because it did not properly support unicode whitespaces (such as the japanese double width hiragana space and whatnot), or just plain the basic ascii form feed.

It may be (arguably) more efficient to boot.
If this is meant to be internal, please mark it as `protected` like you did with the other internal ones.
Use a `static if` here (and add a space before the brace).
The first version's name :) Will be fixed.
Opps. I forgot to un-comment them. Thank you for catching this.
thanks for hanging in there with me with all the renaming. I think, however, these rename iterations were definitely worth it. AFAICT I don't have anything more now. Feel free to rebase and push
it's not really page version, but rather versionOfTheBundleRepresentedOnPage or whatever. Feel free to use longer names if they make your tests better readable!
maybe move declaration and instantiation in one line?
createIf() in hqlutil.cpp will need updating for these to work.
Can we use `:button[name=action_save]` in all JS interactions please? There might be forms with class `.cms-edit-form` which don't have the ID `#Form_EditForm` (for example in CMSAddPageController).
I did the average time per patient by passing in the `Registrar` object. We should probably do one way or another for consistency. Why did you choose to pass in `num_today` explicitly?
> looks strange to me ... 

Not really.

```\``` = Java string escape-sequence
```\\``` = Java string escaped ```\``` = RegEx escape-sequence
```\\\``` = Java string escaped ```\``` (= RegEx escape-sequence) and Java string escape-sequence
```\\\\``` = 2x Java string escaped ```\``` = RegEx escape-sequence + RegEx escaped ```\```
Oh, I see, you mean when checking for the existing tags you are robust against the possibility of a product-specific and global tag of the same name. That's good, and a sensible thing to comment. Maybe clarify that here, clearly I misunderstood this comment, thought you were talking about tag-creation, not checking for existing.
:hand:

I've always felt visibility is of higher importance, therefore I suggest to have the `static` keyword follow the visibility keyword.

On the other hand, it's not something I couldn't get used too.
`authProvider` should not be `null`, and no more checks along this class.
I think this last assertion is the only one you actually want here. Or well, actually, one or two other assertions, asserting that the case was actually created, and that ``result.num_cases`` is 1.
I added a check for sakai.config.enableBranding but I also need to check if the element is visible. On the index page the branding isn't shown even though it's enabled.
Would you mind including the user name in this message for easier tracing later?
Right, good idea.
s/~~need to~~//
I'm not suggesting fixing the schema to make it required, but to fallback to the bean name like we do elsewhere. That's why I provided the code excerpt.
You may want to add the **</para>** tag here and remove the one further below. Particularly **<programlisting/>** blocks seem to exhibit margin-issues when placed within **<para/>** sections.
Reading this comment makes me think. Why not just set default timeouts for @Gateway to:
```
long requestTimeout() default Long.MIN_VALUE
```
This way the behavior is the same but -1 can still be treated as an override and we don't really have to have this comment which at first was a bit confusing. 
We actually have the same problem and the same solution in PollerMetadata so its consistent with other components.
Why you've removed the setters for the class?
Fixing duplicate entries in the menu.
If remote_port defaults to 514, will it ever be ""?  Should we skip line 26 altogether and just use remote_port for line 27?
OK, I like this solution. Though it's still a bit ugly to require of user to specify the url with .configure before he can use this... I will write it so that user can either specify the url in global configuration or pass it to arbitrary method through the options parameter.

On 11. 09. 12, at 11:55, Mitar <notifications@github.com> wrote:

> In tests/test_project/test_app/static/test_app/datastream.js:
> 
> > +            plot.getPlaceholder().unbind("plotselected", onPlotSelected);
> > +        }
> > +
> > +        plot.hooks.bindEvents.push(bindEvents);
> > +        plot.hooks.processOptions.push(processOptions);
> > +        plot.hooks.shutdown.push(shutdown);
> > +    }
> > +
> > +    $.plot.plugins.push({
> > +        init: init,
> > +        options: options,
> > +        name: "datastream",
> > +        version: "0.1"
> > +    });
> > +
> > +    window.datastream = datastream;
> OK. But then you can simply do as jQuey supports. A plugin which operates on DOM objects, and some methods which are accessible through jQuery. So that you can do $('#plot').datastream(options) or $.datastream.allMetrics().
> 
> And you can also then have $.datastream.configure(options) for global configuration. Similar to how jQuery has for Ajax.
> 
> —
> Reply to this email directly or view it on GitHub.
> 
>
Fixed the alignment on this comment too.
When is this used/valid?  Is it `EventSource` only?  I'm not sure how it's valid for polling since a new connection is made every time.
Ahh! Yeah, that makes more sense. I'll fix this shortly.
can u splain this? maybe in a comment in the code? seems dubious w/o no explanation
Yep! Done.
does sqlite not have all of those options, or why the @ notations?
I don't see how this changes any behaviour:

```python
def get_git_version():
  return version

class VERSION:
  RELEASE_VERSION = get_git_version()

with open('content/gitversion.txt', 'w') as f:
  f.write(get_git_version())
```
Yes I think must is what we want here. We want that "amazing" experience for EE :)
It has nothing to do with JMS..
It is a problem if I'm going to have to be testing dozens of these. It also doesn't make us look good. Remember with the type of people we want to hire they are interviewing us as much as we are interviewing them.

So you would rather see someone write the dev test in a language/framework they are just learning then one they have mastered? I think this is a mistake.
you guessed it, 2 spaces soft tabs
Why don't you follow ruby coding conventions?
Shouldn't the code in this module be going through the PatientSummaryService rather than the report definition service?
E.g when looking up report designs by uuid that have been configured in the GP
None of the meaning of the docs was changed in the first commit of this PR - it was just done for reformatting purposes.

SystemButton.yml has been reformatted but not rewritten. Please only review changes made in the last commit - ie, rev1.
re-order these.
We can remove this file, no?
why this ? Let the parent type do the logic about transforming the value
Please make 'default' a constant if you are going to be comparing against it in multiple places
Metody z wielkiej litery (CamelCase).
Remove this empty method