labofsahil/github-event-dataset-2012 huggingface dataset:
1) There can be other needs. For example someone can get task to do filter on executed commands and disallow if command satisfy certain conditions specified in config file. If so, he will need this two methods, but now he must wonder if he can publish them, maybe he will have to look at each descendant class to see if he can do that. This wasn't his task. If this methods were public from beginig everyone who extend GitCommand class has to implement those two methods having in mind that they are public. Here is the same lack of information as in "sealed" case. I don't know if those methods are protected because they have to be protected whether they are protected because they do not have to public. For me this is not the same. 
I miss class methods, that are available in delphi. There you could declare GitCommandName as virtual abstract class function.

2) I dissagree because ToLine is virtual. Even if it was not virtual, it will be still a bad design pattern to extend functionality of class, which we have full access to its source code, by adding extension method. I don't like it.
s/technics/techniques/
49a0d34
why don't we like typekit?
Could you push the CR replacement and file open code into CSVImporter?
Probably rename import_xml() and import_csv() both to import()
That way it would match the XML Importer, and you could probably reduce the repeated code in the
if ($extension == 'csv') else if($extension == 'xml') block. Just assign the correct importer class and reuse the rest.

Going further, both of the importers could extend a base importer class  (or possibly interface / abstract class?).. and let you do something like Importer::factory('CSV'); .. but getting the basic interface the same is more important first.
This file doesn't need to be committed -- it will be generated as needed during the build.
height and top should be multiplied by scaleY
Should almost never have inline styling like this.  Make a specific class and add it here instead of inline-style.
I have a feeling we have more than one occurrence of this in the code, we should make it into a function.
Is `desig:' really necessary here?
@jhinkey Well, auth.pipeline.pre=com.liferay.portal.security.auth.LDAPAuth is set by default, you don't need to change anything. But when you change this property, you must change the other one too, which isn't obvious and IMHO requires attention.

And imagine a scenario - customer removes the default LDAPAuth and implements own one, which may or may not depend on  auth.pipeline.enable.liferay.check (and LDAPAuth). In this case we need to define what it means "LDAP compatible authenticator" and make sure it is set. I think it's too much work and almost no added value.
Because urlencoded values will be decoded otherwise. For example if you have <code>$url = 'www.example.com?keywords=foo%20bar'</code> <code>$result['query']</code> should be 'keywords=foo%20bar', instead you'll get 'keywords=foo bar'
```.to_not yield_control``` wild stuff.
Message written on STDout instead of logs
What is this checking for and when would it be not true? (This is equivalent to `arg == res`, right?)
Can we give a clear explanation of what happens in each case (original is a string, original is a String[])?
We cannot create the inputs via the REST API. I have the app. I need to figure out about installing it. I'll file a jira on myself.
I can see `EmbeddedListField` in `tastypie_mongoengine.fields`?
`from piplmesh.api import models as api_models`
You will have to provide me a bit more detailed information: stacktrace for starters and what I have to do to reproduce the error. How can I help if you don't tell enough?

Otherwise, you can always simply just patch the version we are using. So you send upstream (to Tastypie) patch for the latest version. But also make a patch for the version we are using and use this in the project.
Leave default please?
But I see no problems still? :D I just copied the current version in, and nothing changed
I thought that it wold be a top-level constant in the code, not setting. Because we do not really have code which then adapts the display based on that. But ... we could have it! In fact, we could just change that `range()` call to use this setting, and then we could have dynamic number of forecast, based on days.
That is not good. You have two different elements with the same ID (`browserid`).
Where is this method used? Is it needed?
-            { "staus",          SEC_GAMEMASTER,     false, &HandleCheatStatusCommand,          "", NULL },
+            { "status",          SEC_GAMEMASTER,     false, &HandleCheatStatusCommand,          "", NULL },
Bei Zeiten ;)
Please re-align stars in comment block.
Is it useful to pass the list of nodes and the node_name at the same time? Shouldn't we directly pass the node information we want?

If not, shouldn't "node" be "node_name" or "node_identifier"?
from Github account?
It would need to be on the response, then it could be cached in the getter, at least as soon as it has a responsebody.
Sidenote, I thought unfinished / body-less entries wouldn't make a ResourceInfo, but it seems that's not true?
How could there be no `cls.ResourceUtil.guess_type`?
Non-portable (though most major compilers accept it).
Probably because I wrote that line first and missed it in the refactoring. Thanks. :-)
Likewise, you don't need these reshapes

```
beta = np.arange(1,5)
y = np.dot(X, beta) + np.random.standard_normal(40)
```
Don't remove copyright, update the header, e.g. to

Copyright (c) 2010-2012. Stephen Connolly, Henry Gomez.
could we just set `this.media.poster` to `''` if it's not defined? that would avoid some duplication here.
Typo: beanstlkd => beanstalkd
name it archipelguest or something otherwise it would be confusing
You certainly need to import "TNLibvirtDomainQEMUCommandLine.j" here
Well the first sentence is definitely obsolete, but is the whole para really obsolete?  The sentences beyond the first seem okay to me - I pushed a version that eliminates the first sentence and tweaks the others a little in a way that seems okay to me -- i.e., clarifying that they are what happens *if* you supply an input image -- but please confirm.  (If they are actually obsolete then I think I've missed something!)
I'd rather not do a big refactoring at this point; let's talk on the call.
I'm actually not sure how to do that...
Look into using a nested resource for devise

try 
Or you can migrate the devise controller and add it as a method to the devise controller
devise_scope :user do
  get "dashboard", :to => "dashboard#view"
end
this code hasn't changed but i've swapped it to underscore which may be more portable
Like this?

```
If the value of the named property is a function then invoke it; otherwise return it.
```
Is there a case when we will need to return the image data in a `onFailure` response? This code could probably be simplified if we didn't have to support this.
This should be in a model, no matter what it does, something like `Refinery::User.with_post_counts.all` or something more clever.
The Cygwin/MSYS launcher "sbt" should probably use the same settings as "sbt.bat"
if you move the `if convert?` part back here, so

    if convert?
      value = convert_units value, row
    end

then somebody who is not using unit conversion at all will not have to define a unit conversion library (preferable)
чому 16?
Is exlcude a typo?
ok. fair enough.  You might want to freeze all the constants though then you will also get enforcement of the convention.
comment on why?
heh but I like the least amount of settings?! :)
I think you can also write these queries like so:
```
Relationship r1 = Relationship.find("byToAndFrom", user, other).first();
Relationship r2 = Relationship.find("byToAndFrom", other, user).first();
```
we're reusing the client object that already has the curl options there -> https://github.com/arturo-c/USARugby-Stats/blob/b79b87dd3144928250653649099024a7d34862be/app/index.php#L144
I'd like to move this to a factory instead of making the constructor so busy.  If we have a getClient() method after the object is constructed, we can add any plugins we want in factory methods.

Also, $curl_options is confusing because it only appears to be used for basic auth, when you can actually pass a host of options to Guzzle.
What do you think about extract the method below to another utility. The usage could be something like JsonBlah.parse(responseAsString);

Might be useful to another classes.
no UMD rely on this to display merchant name
nvm the above.
NullPointerException - результат ошибки программиста. Везде, где null возможен - надо проверять на null, а не ловить где-то NullPointerException.
А если число очень большое?
Any reason to expose a callback here instead of having an event that we can attach listeners to?
`Target.jsm` no longer has a `Target` export, looks like you didn't need to import it after all anyways.
why can't you use `is_element_present()` here?
@klrmn is right, you don't need the init method here.
We should use "filter" (no plural) since only one filter can be applied at once.
細かいですが、最後の「:」について、日本語で不自然だから省いたのか、原文の記号は保存する方針にするのか。
Should probably remove that Log when we aren't enabled.  (we want the app to be quiet when it is installed if turned off)
You simplify the whole method by directly returning `np.average(...)` no matter the value of `sample_weight`. (As I did with trees.)
sorry, forget about it.
I added a little spacing, but I'm not quite sure what you would like to see.  Perhaps the LaTeX-style definitions I gave to the other `_em_X()` functions?
this seems a little bit odd to me. the class is a "Song" (singular), but you initialize it with a lot of "titles" (plural).

so, yeah, it would make sense to just rename this into "Playlist" or otherwise make it so that the jukebox initializes an array of songs where each song only has one title?
When I say delete the entire temp I mean delete the entire temp for a site not the TEMP folder itself. It's still wrong for something to clean up what it didn't create. 

I'll just change it to have everything in the kudu folder instead of having a folder hierarchy.
OK. It is not a strong argument. You can keep original code style.
Why is this indentation off?
I'm not sure about the static's here. Multithreaded should break.
why two empty lines? could you remove one?
I misunderstood the code.  See comment on addHash below().
I'm sure this will leak.  It should be loopInputBuilder->queryrows()

It is a helper function which almost never assume ownership of  a rowset
Do you need these comments?
Here, too, a regular expression would be far better
The example is for a hash aggregate rather than a hash dedup.
Your patch is to fix this, right? To make ds1A have the same flags as ds1_dist, ie. local.
Because normally you would be setting packed automatically if a downstream activity (e.g., sort) is going to hold onto the rows.

It should be based on that flag though, not on whether the allocator is packed.
I think it probably should be a memsize_t, but I would leave that until the change that other change is implemented - since I suspect there are other places that also need to change.  (E.g., the parameter to this function).

It could probably be a RoxieHeapFlags, but I suspect the same is true of the other functions in the interface.  I think it is that way to make it easier to pass values to.
Could say 
    ::Release(spillBuckets[i]);
My concern is that entries in the hash table may be in the wrong place. Perhaps that can only cause false negatives, and perhaps you don't care about them?
Is it worth special casing which==slaves, rather than always doing the scan?
should be const char * variables
Can we change this to:
```python
def getAfterburnerCoordinates( chain, mfc, n)
```
And have have another function
```python
def printAfterburnerCoordinates( chain, mfc, n) :
    print getAfterburnerCoordinates( chain, mfc, n )
```

If you would like to keep the functionality!
#!/usr/bin/env python needs to be at the top, so the shell knows what interpreter to run (it defaults to /bin/sh):

./manage.py runserver
Change this to 
```python
def __init__(self, current_versions, *args, **kwargs):
```
If you take my advice and let `urlencode_params()` take care of all input to `get()` you can do this instead:
```
api.get(
    ...
    start_date=start_date.date(),
    ...
)
```

It doesn't need to become a string until it goes out on the network.
@rhelmer I am actually referring to the view tests inside test_views.py [https://github.com/mozilla/socorro-crashstats/blob/master/crashstats/crashstats/tests/test_views.py#L138]
These aren't necessary; the import statements are enough.
does JText::_('COM_KUNENA_CATEGORY') work?
The changes start here
Already registered by HttpServer. If you just want to change the default, use ```"DefaultOptions" => { "SRVPORT" => 80 }``` in update_info
Couldn't you use the %PROGRAMFILES% environment variable to get the "program files" dir path?
There unfortunately is nothing of interest if RememberPassword isn't set but always good to question it. :)
please remove dead code if it is no longer needed
You're right, it needs to be directly assigned back.  I'm working on a better way to deal with this in general, though, using data passed as an array rather than a list/iterable (as recommended above).  I'm getting ready to push a revision that addresses most of yesterday's comments.
This doesn't seem right--why take the floor?
Alrighty, I added a test case to check that the add_role_to_message method returns the message unchanged when Rails < 3.1.  Then I did regression testing from Rails 3.0.3 to 3.2.6 via explicitly specifying the Rails version in the Gemfile.  I ran the full test suite on each, using the previous test cases to prove that the matcher still worked, and my new tests to see that the messages were appropriate.

I also built a quick application and test suite with Rails v3.0.15, just to be triply sure.

I didn't see any issues anywhere...
It looks like that your editor uses Windows line endings. All of our repositories are based on Linux ones. So please adjust your editor accordingly and clear this change.
Instead of declaring `self`, you could bind the function here:

```javascript
this.curRetryId = setTimeout( _.bind(this.dataLookup, this), this.retryDelay );
```
Sounds like this is hard to do reliably.  Since I don't expect this to be a common mistake wen authoring Fabric, I'm OK with just removing the check.
This isn't really related to your change, but I notice that you are doing "this.material._getShaderSource()" here instead of "this._material._getShaderSource()", which can result in calling a method on 'undefined' because on line 512 you default to a new ColorMaterial().  Even if you change it, the code is still bad because it ends up creating a new material every update if this.material is null or undefined.  You probably want to change line 512 to assign new ColorMaterial to this.material and this._material
Could use an `@example` too.
Okay, thanks.  Didn't know that.  I just submitted a fix.  I expect all of the material stuff to change dramatically once @IanLilleyT stuff is in master, so don't spend too much time worrying about it.
Should this be here ?
Please use a var for 2 instead of the "magic number" ;).  Or a comment.
Well, in my understanding, an href is an attribute referencing a hypertext document from another document. In this context, there is no reference but just the information where a document can be found... It's a difficult question but I just haven't seen *href* in this context yet.
You should rename this to README.md, which allows some minimal mark-up
these are folders, not files
Yes; although I changed this for consistency with the commenting style in drupal-org.make (where the modules are downloaded).
Tipo, esses testes seria uma boa pratica tu dividir eles em 2. Um pra testar se a instancia e da classe correta, primeiro teste, e o segundo pra testar se o valor incializado e correto. No final das contas estao ocorrendo 2 testes aki. A unica diferenca e que voce so esta descrevendo um deles ou os dois de uma maneira mais generica. Se voce usar um it com desc pra cada, quando quebrar fica mais facil perceber o que quebrou. =D
It's not mixing, I did it with a reason here. I pass `undefined` to a callback instead of `null`, because I assume that `void` == `null`.
А я могу ему сделать `new`? Нет? Тогда ты full of shit.
The clause with "but" in it is not independent, so it's not valid ([quick explanation of clauses](http://www.learnenglish.de/grammar/clausetext.htm)). I'd probably phrase this something like

> This makes $(D popFrontExactly) faster than $(LREF popFrontN), but it also means that if $(D range) does not contain at least $(D n) elements, it will attempt to call $(D popFront) on an empty range, which is undefined behavior. So, only use $(D popFrontExactly) when it is guaranteed that $(D range) holds at least $(D n) elements.

I worry about using safe in this context simply because @safe refers specifically to memory safety, which tends to overload the word safe in discussions on D. So, I tend to try and avoid the word safe when not talking about memory safety.

Also note the use of `LREF`. You should use it instead of `D` when referring to another function in the same module (or `$(XREF mod, func)` when referring to std.mod.func).
Same as `visit`. Instead of  `Delegate`, more generic name is better.
Please change this to use `takeExactly`. As `Sequence` is an infinite range, it's guaranteed that there will be more than the number of elements requested, so `takeExactly` will work just fine, and it has the advantage of having `length`.
Why the std.stdio? That should be completely unnecessary, and since `writeln` is pretty much never used that way, it makes the code harder to read.
Repo. Or maybe make some gh-pages?
Good point, it should have 2. Fixed in 4249614 - thanks for the feedback
Reusing the `buf` variable here instead of creating `du` will probably better garbage collect
If a parallel build works, just remove the line above entirely.
Because OSL doesn't directly reference or link against IlmImf (the OpenEXR file format), only IlmBase (for Imath stuff).  I think they weren't used before at all, and in any case the variables they set are no longer referenced now.
The patch is used in the script `update-bootstrap.sh` to make updating to the next release automated—yep, it is intended to be committed.

As I said in the pull, I'm open to other methods, but I think automating the procedure you'd otherwise be doing manually is a good solution.
this is no longer needed and can be removed, or?
Please. Will leave note inline until core can accommodate this.
shouldn't that check stop the whole machinery from being bound (iaw be in the module) ?
I do it manually in the configuration of the service bus in my client code. Otherwise when I do `.UseAzureServiceBus(cfg => cfg.SetReceiverName(...))` or something else manually, this line ends up taking precedence and my manual service bus config is ignored.
> My plan is to add the time span feature (from, to) to generate the demo data in a certain time span

You mean dummy data.

So, demo should work like this: user clones repository, runs `--demo` and runserver, opens local host in the browser and sees graphs full of data, with new data streaming in all the time (this will be done through HTTP push server, which we will integrate later).

> add a clearstream command which will remove all data from the database.

Name it "flushstream". This is what Django uses for flushing database, too.
замыкание? это так оно называется на русском? фу
Why has the movie link been removed?
Unless daemonize is set to 1 in server.cfg starting the service with this service file will hang. If the type is set to simple it behaves as expected, even with the default daemonize of 0.
`false` why? in config.js.ic is `true`
What is this warning? What does it mean? Is it still valid?
Yes you missed, maybe it wasn't clear ... I can't write what you propose in the template but I can write sonata_render_block('../footer') (or whatever the name of the twig extension is ...)
once more - you have:
```actionscript3
if (array.array.length < (offset + (int (array.array.length / stride) * stride)) ) {
```
let's omit int(...) for the moment:
```actionscript3
if (array.array.length < (offset + (array.array.length / stride * stride)) ) {
```
now / stride * stride:
```actionscript3
if (array.array.length < (offset + array.array.length) ) {
```
now subtract array.array.length from both sides:
```actionscript3
if (0 < offset ) {
```
so what he's saying here is that if that int(...) part is meaningful, you should revise this if (...) to make it clear.
Consider moving the final iteration outside the loop to avoid a conditional inside it.
I'm not sure I agree.  If someone needs it later, they won't know to look here instead of looking on a more generic utility class.  According to the KML spec, `when` is the contents of  `TimeStamp` which can be part of any element deriving from `Feature`. Better to move it to utility now so that if someone else adds a feature that needs it, they can easily find it.  Better yet, you might want to make it an Extension method of JulianDate.

I've looked at the code a little more and found you already have a `Utility.GetInterval` and `Utility.GetTimespan` that are doing their own `when` parsing.  All of this code should be refactored so that anything dealing with time is using the same set of utility functions.  KML is already defined so that all `TimePrimitive` elements behave in standard ways.  Therefore, we only need a small set of reusable functions to deal with them, all of which can live in Utility.
Why move this import around? Now it's no longer sorted.
I believe we can't assume `Array.prototype.push` exists because of some old browser that doesn't include it. Probably IE6. Needs verification.
que l'on est pas sûr, que l'on a été
using underscore's pluck to find out which corpora are already in the user, via their pouchname, if this one is not there, adding it.
I've removed them after Emmanuel had expressed doubts that it's not clear whether the "<" and ">" are parts of the actual String or if they are used as delimiter in the syntax description. To express that _PARAMETER_INDEX_ is a placeholder I've used italic font for it.
-1 for the indirection
This works in the native app, but not in a hybrid app.  I'm kind of surprised it works in the native app, as `NSLocalizedString` is meant to find strings in the main bundle.  I think you'll want to use `NSLocalizedStringFromTableInBundle` and explicitly define the NSBundle, based on the new bundle file.
Corollary: why use a `<button>` if its role is to be a link?
Can we make our own namespace for our stuff?  In the past I've done something like:

ecep = ((typeof ecep == "undefined") ? {} : ecep);
ecep.init = function(mapid) { ..

or even better:
$.extend(ecep, { init: function(mapid) { ... }, loadLocations: function() { ... } })

Maybe this is overkill for this project, but it helps avoid global namespace clutter
Should this be 5 or 10 seconds?
Do you really need the _Action_ part? _IndexingInterceptor_ sounds nicer to me and imo _Action_ is so general that it hardly adds value. If you want to add something additional why not _IndexingWorkInterceptor_. AFAICS the intercepting occurs on the during _Work_ processing, so adding _Work_ seems to add more value than just _Action_.
assembed?
You mean the `d-nb.info` URL?
contentTypeSelect is now built with a forloop...this was the main culprit for why the javascript could find the inlines if they were linked via ForeignKeys but not if they were linked generically.
This appears to be unused and may need to be evaluated for removal.
This is actually not listed in tiapp.xml, but is built in as part of the SDK
на пам’ять залишили?
Хотя, может метку, что пост удалён не убирать? Пусть юзер видит в таблице скрытого, что пост удалён.
I still suggest moving Webview into it's own file (webview/Webview.js or something) to shorten this module and increase readability, specially with all the comments (based on my experience). :-)