labofsahil/github-event-dataset-2012 huggingface dataset:
Ok... nothing about this change means we can't always have code-gen, though some projects that are migrating can't use codegen - this means they have to do a big-bang push.  With this structure, we can privilage code-gen for all three kinds of handling, but have other fall-backs.  In the case of injection, that's reflection.  In the case of module handling, that would be things like a guice module handler. 

Yes, this is how dagger works - but allowing people to go off-warrantee in order to have a path to GET to those benefits that doesn't involve a re-write is important. 

Validation should be default-on.  But allowing someone whose code can't be validated explicitly opt out to be able to get SOME of the benefits of Dagger while they restructure to get all - that's not a small thing.
Note that this is structurally different from AOSP (which doesn't have SPDY).
http://omapzoom.org/?p=platform/libcore.git;a=commitdiff;h=b7705a4629bb74a5bb19d97bf0b4dd4c74cecd9e
This is going to screw up the changes I made yesterday with the image embed plugin. This is after you merged master into your branch already?
Sounds fair enough. I'm still in favor of using Hamcrest though because it's already build-in:

Compare

```java
assertThat(x, is(lessThan(y));
```

to 

```java
assertComparable(x, LESS_THAN, y)
```

I would prefer the former version as I find it more readable.

@kcooney: Which would you prefer?
if you mean the part on bottom "org.codehaus.mojo", then
it's the plugins section. 
the plugins are responsible for making the build and used by us, not by users.
plugins are executed in a phase of build life cycle. The phase can be specified as default by the plugin itself or you can override the phase for the particular plugin.
http://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html

if you mean the extensions wagon-http and wagon-ftp, then i explained before. They are used in order to upload to a remote repository via http and ftp protocols, respectively.

did i answer your question?

The Apache and Sonatype published very good books for maven
http://www.sonatype.com/books/mvnex-book/reference/public-book.html
@terraflubb the name may not be the clearest, I admit.  `canSetImplicitly` means "can we implicitly set the value to the first option?`

It's used to make sure we only set the binding to the first value in the options list, if the value we're trying to set is `undefined` or `null`.  In other words:

`dataChange` is called when the binding changes.  The first thing we do is check to see if the `keyContext` has changed since the last time `dataChange` was called.  The `keyContext` would be the prefix on the key path before the actually key value.  For example: `foo.bar.beans`, `foo.bar` is the `keyContext` and `beans` is the `key`.  

`canSetImplicitly` starts off `true` and is only set to `false` if either: 

* the value we're trying to set the binding to (`newValue`) is `undefined` or `null`
* the value is specified

If the value is `undefined` or `null`, we wait until the `@node.value` is present (ie. we have at least one option) before setting the value to the first option. 

If the value is specified, we grab the option that represents the `newValue` via `valueForNode`.

`canSetImplicitly` is only ever set to true on the first binding or if the `keyContext` changes.  Since bindings are set up when the view is created, they stay around for future renderings of that view.  This mans that `canSetImplicitly` would only ever be `true` the first time the view is rendered.  We ran into this bug when view caching was introduced because the bindings aren't recreated, only their `keyContext changes.  That's where the `lastKeyContext` check comes in to play.  If the `keyContext` changes - often due to something like binding to a different model - we then reset the `canSetImplicitly` boolean and proceed as we did the first time.

I hope that made more sense?
the issue that the spout engine called the getWorld() function but because the instance of the engine used by the client is a spoutClient wich is a subclass of spoutEngine, so its this function that is called, but the function that call it precisly the function that set the activeworld so it'll always be null before that. And it doesn't make any sens. Also this function have no other interrest for now so it can be just commented.
os = "desktop" seems a bit weird to me. Maybe this should go under environment?
With this moved out, I think you can remove the 'var self' in this function (the only other usage should really just be 'this').
Should move this up so it's next to getChecked()
If user installs Brackets on a different drive than the user folder, then there won't be a relative path.  The simplest example would be to install Brackets on something other than the C: drive on a Windows system where user folder is in default location on C: drive. Will this cause a problem?
Shoot. That means the ``indexOf()`` in ``removePopUp`` will fail too. Looking for an alternative.
Keeping this change until someone needs it. Renamed autoAddRemove to autoRemove.
Once you make the change above (removing _colorPickers), then you can move this function's definition directly onto InlineColorEditor.prototype, which I think is a cleaner place for it.
Hm, what's this about?
Application sets the log provider here allowing them to use whatever logger they wish.
I think it may be worth elaborating what the expense is
Can this last if statement be removed, so the last return statement always happens when the previous `if` statements are passed over?
could/should we condition this for 1.8 only?
Oh, I didn't know we need to register every package. The reason I did that was I had another Mr. IBP (MapReduce Indian Buffet Process) package available, and those code blocks were shared among these two. It is fine for me to put those back to the cc.mrlda.util package.

Ke

On Mar 18, 2012, at 8:29 AM, Jimmy Lin wrote:

>> @@ -1,4 +1,4 @@
>> -package cc.mrlda.util;
>> +package cc.common.util;
> 
> Why moving into cc.common?
> Shouldn't everything be under the mrlda.cc package hierarchy?
> We don't own common.cc, so it's bad practice to put there...
> 
> --- 
> Reply to this email directly or view it on GitHub:
> https://github.com/lintool/Mr.LDA/pull/2/files#r572091
There was already before the problem with this approach: race condition. What if somebody creates an user object just between we check for existence and then go and try to create it? Can we do something about that now that we have moved to MongoDB? `get_or_create`?
Change to updateScrollbars
same as above, this should already be fixed. might merge cleanly though
Looks like `__mouseDiff` is always `0`.
Either what's mentioned above or these aren't updating them properly.
if there is a plan to have abort, let's add `'abort' in obj` check. Otherwise let's call `done()` here.

also, shall we destroy page here as in previous case?
Thanks very much. 

I've got an OSX machine here so I'll be sure to test any changes I make from hereon out. Though I think the C code is safe now :)
It's not one of my update. I have just move some code.
Can you add something like this to util.py perhaps? `PY3 = sys.version_info >= (3,)` Then we can use that instead of writing the longer test.
I'd generally prefer to not cram in too much in addition to the method declaration on one line
Good catch, I will fix.
`allows you to filter...`
This file needs to be renamed `index.html`
seems there's a typo here, should be `convert_seconds_to_timedelta` I guess.
Do you mean distutils instead of sphinx?
I'll add, an additional advantage to this approach is that it supports thread and process-based executors with the same API; the cone search executor could optionally use either one of them.
i would perhaps name this method `create_remote_source`, which seems more consistent with the documentation
Agreed. This was there already, but got moved with my window rearrangement (see 'old' line 82 below).
It shouldn't be, thanks for noticing the bug :)
Yes,

    hSocketMax = max(hSocketMax, hListenSocket + 1);
    ...
    hSocketMax = max(hSocketMax, pnode->hSocket + 1);

Then change the select line to (ie, remove the + 1)

        int nSelect = select(hSocketMax, &fdsetRecv, &fdsetSend, &fdsetError, &timeout);

And change this loop to < i.s.o <=:

                for (unsigned int i = 0; i < hSocketMax; i++)
                    FD_SET(i, &fdsetRecv);

Only after that, you can check for hSocketMax==0 and be sure that it means "no sockets".
Shouldn't the method in the form tag be changed?
you should rename the property, and declare it (undeclared properties have a performance impact as PHP 5.4 optimized the property access for known properties)
Extra space after the first opening parenthesis
you should remove ``$element``. There is not variable name for a return statement
I probably would use an active record class, but I'd define it in the migration class. Eg:

class Event < ActiveRecord::Base
end

I typically try not to add anything else to such classes though (no associations), because then I need to define those in the migration too. So, I tend to work with the raw columns in the migration, such as event.comment_vote_id or whatever.


On Wednesday, 8 August 2012 at 11:37 AM, jonlemmon wrote:

> In db/migrate/20120719062306_add_eventable_to_events.rb:
> > + event.eventable = CommentVote.find event.comment_vote_id > + end > + event.save > + rescue ActiveRecord::RecordNotFound > + event.destroy > + end > + end > + > + remove_column :events, :discussion_id > + remove_column :events, :comment_id > + remove_column :events, :motion_id > + remove_column :events, :vote_id > + remove_column :events, :membership_id > + remove_column :events, :comment_vote_id > + end > +  
> Interesting, how could you write a migration like this without using active record classes?
>  
> —
> Reply to this email directly or view it on GitHub (https://github.com/enspiral/loomio/pull/246/files#r1327797).  
>  
>  
>
I precise : not directly in the client
We zitten hier in een model, dus enige alternatief is.

BaseModel::getContainer()->get('database')
__exist__ should bne plural (exist__s__)
This type of if statement only needs to be on the base classes.  I would just put the code in this block at the top level and get rid of the test.  See `Cell` and `TextCell` for an example.  I know this is very weird.
see, you already fixed pep-8 here:-)
Is there no way we can recover from this?
Can we perhaps use generics instead of so much code duplication?
can we move this out of get?
as you mention in your first comment, it would change the performance of any current use cases (where there are no wildcards) by processing any additional non-indexable fields. 

Your second comment is interesting. Until now, I hadn't really thought about multiple regex matching the same doc field. My assumption was that it is probably an error. Given that the order of how these fields are matched is not specified, can you think of any use cases where we want to have two separate regex fields rather than collapsing them into one?
oh boy, `&` and `&=`. I think it is the first time I ever see this in python :-)
We make use of the 'got - expected' style. So would be nice when you can change that message.
Again, a very specific error message in a very generic "except" - there should be a much more specific exception caught here if all you're trying to catch is "RunCaseVersion not found"
Ale to je spíš poznámka k eval systému, než k tomuhle.
а без цього ніяк? в мене працювало без data_class і т.д.
This is wrong. refreshUser will accept any user, even if the username is invalid. It is a security issue
`join` or `implode`? 
@stof Is there any CS rule for this?
Symfony does not wrap the constructor arguments.
just so the different test cases are easier to distinguish. is this now allowed?
I agree with @Seldaek
``function_exists('posix_isatty')`` will always be ``false`` on Windows as the POSIX extension does not exist there
CS issue. ``else`` should be on the same line than the closing curly brace
Does this fly as a single line?
More like:
```
def invitation_message_body
  ERB.new(
    File.read(
      Rails.root.join(
        'app', 'private_messages', 'users', 'invitation.erb'
      ).to_s
    )
  ).result(binding)
end
```
in raveshe dorosti nist
eslahesh kon :D :trollface:
if you want to be very specific, you could say:  tested on - Ubuntu 10.04, 11.04, 11.10, and CentOS 5 and 6.  since we've tested these all with the sensu-tests repo
Don't use processes for this. It's error prone to spawn processes, the forking can easily fail if there's not double the amount of the agent's used memory free. It's ok to use threads for this because the socket calls are IO bound, so the effect of Python's global interpreter lock is negligible.

Also, you should use a proper thread pool, there are many already implemented, like this one:

http://code.activestate.com/recipes/576519-thread-pool-with-same-api-as-multiprocessingpool/

Thread pools are important because it puts a limit on the concurrency so that you don't kill the box if you define too many checks. It'll do N checks in parallel, allowing more as slots free up. It's also simpler to reason about. It's not clear how this concurrency works.
Sure, I can add that, I see it being useful.  The only ways we used it at the moment just wanted the string.
I think it'd be best to avoid the network/server dependency altogether. If nothing else, I should be able to run the Cartopy tests successfully even when offline. But rather than some cleverness to disable/ignore the test when the server/network is down, better to just fake the server in the first place.
Last minute change. Oops.
Maybe we should do np.unique to make sure people don't ask for the same state twice?
авыавыаы
That's just how I roll for methods that take *args ;)

I'll fix it
Bien vu.
On Wed, May 2, 2012 at 11:46 PM, Brian Stansberry
<reply@reply.github.com>
wrote:
>> +    /**
>> +     * Returns "secondary.test.address" system property if such exists. If not found, then there is a fallback to
>> +     * {@link ManagementClient#getMgmtAddress()} ("127.0.0.1" is used in case mgmtClient is <code>null</code>). Returned value
>> +     * is formatted to use in URLs (i.e. if it's IPv6 IP address, then square brackets are placed around - e.g. "[::1]")
>> +     *
>> +     * @param mgmtClient
>> +     * @return
>> +     */
>> +    public static String getSecondaryTestAddress(final ManagementClient mgmtClient) {
>> +        final String mgmtAddress = mgmtClient != null ? mgmtClient.getMgmtAddress() : "127.0.0.1";
>
> Why would mgmtClient be null? If it's ever null, sounds like something is wrong with whatever code passed that. And then that test will fail when we run it in an IPv6 environment.

It's an helper method, which can be also called with the null
parameter - it's a correct state mentioned in the JavaDoc. The main
purpose of the method is to return "secondary.test.address" system
property value, if such exists.

> AIUI any hardcoding of "127.0.0.1" or "localhost" is forbidden in the testsuite/ modules.

OK, I'll replace it with the loopback address provided by java.
Зачем not found, а не 403? Можно бросить AcessViolationException чтобы получить страничку о том что прав нет
Oh... slipped. Sorry, changing now.
it is on purpose (much less flickering), and the comment suggest to load l10n resources *synchronously*, hence this `gAsyncResourceLoading = false` statement.
Not exactly: type="submit" can be used with any `<button>` element, so we don’t need the `button.accept` selector here.

These accept/dismiss classes would be specific to `<a role="button">` elements.
By convention we used to apply the following formula:
font-size: -moz-calc( 'size in pt' * 0.226)
for having a clear way of retrieving this value directly. We decided to put in that way because UX make changes often and It's quick for searching and replacing, but once It will be a well defined value could be added directly in order to improve performance (less 'moz-calc' we have, more performance we get).
We should ask UX if there is any other behaviour as in Dialer, but by now it's enough.
nit: the indent seems weird.
Don't leave dead code or explain why you are doing it.
this should be done with class toggling
Is 'shake' really the name you want to use for this animation?
We should rename devContact to something like dContact in order to not to confuse it with the attribute in the class.
Easy enough, done.
I don't think we really need a function for that. I just wanted to keep the current public API:

https://github.com/ttaubert/gaia/blob/24f264ccb13a74f9bf49ae7d4d7ec780cd835a6a/apps/system/js/cards_view.js#L561

Should I remove this? I don't see any clients of this API anyway.
There’s a better way:

```ini
# Prompt that appears when a user tries to delete an app.
delete-title=Delete {{app}}
delete-body={{app}} and all of its data will be deleted.
```

and

```javascript
// If launch_path is undefined, the app icon is actually
// a website shortcut, so use different strings in that case.
if (app.manifest.launch_path === undefined) {
  title = _('remove-title', { app: app.manifest.name });
  body = '';
  yesLabel = _('remove');
} else {
  title = _('delete-title', { app: app.manifest.name });
  body = _('delete-body', { app: app.manifest.name });
  yesLabel = _('delete');
}
```
Hi Fabien,

This config. file includes the DEBUG flag and the constant for the autoclose time on dialogs as specified by the STK specs. If you consider it should live inside the code is ok for me but I think that a config file is more flexible.

Regarding debug lines with JSON striginfies should be removed later ... now is maintained because debugging of this feature shall be done in Brazil and is nice to have detailed traces when vivo local guys send us this traces :p
This is to bypass the Icon code that normally reads the image from the manifest and tries to load it with a URL instead of the data: url version. So instead of trying to render the icons from a remote source it uses the locale database result.
lol ironic.
Also, commhistory-daemon is making use of this function (but it currently uses libmeegotouch). It could make porting a little difficult.
I am not sure we have much of a guarantee that future versions of the GitHub api will use media types of the current pattern. Given that, it might be clearer and simpler just to have a lookup table for for the various media types Octokit supports. That would also allow this method to provide good exceptions if invalid options are are passed.
Yes, makes sense to use the same interpreter again. I think I just changed this to see if it would work.
Can just return a deferred instead of using inlineCallbacks.
Can potentially throw an out of range exception.
1. ah, it's codeplex format, ok :)
2. then you can't take ITracer in the constructor, in case you need it. (see other comment), or should I pass it as an argument to TryGetRepoInfo?.
Yes. The stub is unnecessary.
I wasn't sure if `sleep` would be a problem since it would slow down the test suite.  I'll give it a shot.
> What confusion have you seen it create?

Methods with side effects - difficult to track down bugs. Probably not an issue w/ side-effect free functions.
What about something like `@index.each { |args| yield *args }`. It doesn't seem necessary to expose the detail here that there is a key/value for each index entry.
who would be using it (also clearly you can get at it if you really need it)?

I'm trying to keep a minimal API in grouped. Once we add functions, we should be very careful about removing them.

This method is only here for the temporary bugfix. The real fix (when I added with a TODO) is to handle the Grouped reduction at the same time as the custom joiner (so you only have one shuffle, not two as the current code will have).

I don't think this should be a method of Grouped, but if you insist, I will put it there with a package private modifier.
This feels a bit more generic than Joiner. Maybe we create a CascadingUtils class? Or TupleBase?
Use `HelperFunctions.write_file`.
No this is the old version.
Remove the "-" before parameters to be consistent with other code.
This is a bug of phplangeditor, in the comments (#) adds always = undefined, I usually check it before commiting, but sometimes escape me. When a comment ist, is not problematic
Also this variable can be replaced.
Yeah, something like that. Just for the purpose of doing the user input on this form, I think having a (small) class called MeasurementType with a 'type' field (e.g., ping) and a dict or something representing the parameters, containing the descriptions of each parameter. Your form generation code can easily iterate over the dict's keys to derive the measurement parameters you need and populate the form with the description. I don't think this needs to be super complicated.
So, this PR shouldn't have been merged then?
There should ideally be a refactor so that instead of paused and trigger, queues just use waitFor like Sburb does. But I didn't address that this time.
Don't commit useless files.
Is this necessary any more? Presumably now if the artefact isn't available nothing's available and we should return a 404?
Been meaning to do this for a while, done.
this is very bad; Your distributing internal node-knowledge over various places; Usually the FeedbackTileConfiguration needs to have functions:

public static FeedbackTileConfiguration load(JcrNode node);
public static void save(JcrNode node, FeedbackTileConfiguration);

so you center logic and identification to a single place;
It almost feels like there should be two sets of developer documentation. This is an implementation detail, and people *using* the api shouldn't have to know or care about this (and if they do, it is a bug). And we don't want this fact to be part of the documented interface.

I'm not sure how best to handle this. Perhaps something like the cpython details, that exist in the python documentation.
You could do:
    return self.codebases[codebase].get('branch', None)
... or if there's any corner case where codebase might not be in the dictionary:
    return self.codebases.get(codebase,{}).get('branch', None)
Added a test to confirm the expected AttributeError (with lots of help from Dustin :-)
This comes from https://github.com/cloudhead/less.js/commit/1857b7c5aa92fd5c9454cb4507d688888cba7579#L0R763
Why did you remove 'CCEditBox' and 'network' from extension template?
class and method `{` should be on a new line.
Either $downloadPath should not be passed, or you should pass it always and reuse it in those installCode/updateCode/.. functions instead of requesting it again.
That makes sense - thanks for the tip.
Actually, not complete bollocks.

Those two dummy prev/next links *do* go somewhere, just not on pageload. This module has a function to update their `href`s depending on how you navigate. On page load, the `prev` link doesn't go anywhere (or indeed, appear), but if you scroll forward one image, it appears, and is rewritten to point to the right place. Seems a bit pointless rendering it as a fake, hidden link to then replace it with a real one as soon as the user interacts.
Yeah from my Java dev experience, but Eric pointed out in other places that I should store in a pointer and check for null first, but missed this one here.  I am going to change that here also for consistency.
The original code used both the key "CurrentRevision" and the key "sha".  Can someone clear up the difference, if there is one.  If there is not a difference, we should fold this field into the sha field.
As I understand it, the event stack won't be empty until precisely the moment that all async side effects are done.
Formatting comment as before.
We should have new line at the end of the file – unless is not specifically required by the tests we're doing. Same for the files above.
It's optional, it will instance anyway.
Does the callback get called multiple times and must be aborted?  I'm not clear what this does.
This is why I am wondering if it makes sense to push the user.checkEmailIssuer call into user.addressInfo, it is called twice directly upon returning from user.addressInfo.
Yea, when this view is rendered before the dialog shows, so we won't have the IdP name yet.

The email that the domain is received from had to first get through `validation.email`
When load tests are being run on stage, this substitution will take place for every request.  Is there any performance penalty in creating the regular expressions for each substitution?  This used to be a problem in browsers when RegExps were used in tight loops (DOM lookups, etc).  If so, you can cache off the regular expressions so they are only created once.
So with all the template entries we have right now, which height of the window do we get? I just want to be sure that we do not extend the screen height of the users machine. At least not for a resolution of 1024x768.
This will not work if the function is defined on the root environment rather than the current environment. The existing methods here are inconsistent about that so it's hard to know how it should work. I have a fix in the pipeline for that.
This code block must not be getting run (`obsidinterval_list` empty) because `iexptime` is used but not defined.  Something is strange here.
Yes...definitely better to have the '0' in there, and I usually do. Sorry. :)
/Users/jcheng?
This should be adjustable. Consider an api that uses custom content types; application/vnd.xsdDescription+xml
fixes [ZF2-354](http://framework.zend.com/issues/browse/ZF2-354)
This has moved to the zf2-documentation repo.
This breaks the recommended limit of 80 characters, please revert this again, there is no need to move it to one line. Same for the other occurences where you changed it.
Yes, we ca use php as namespace. Fixed.
I removed all "@return void" from constructor phpdoc. we should check for other components, as well. I saw that all over the place.
CS, max line length is 120, aim for 80.
I see. I tested using rand(8, 4) and with rand(4, 8) the first only show values between 6-8 and the second show values between 4-8

What I want to say? I want to say that the order matters so, IMHO, you have three options:

* throw an exception when the values are incorrect
* Guess the best order for the parameters and then change the docblock indicating the order doest not matter and the arguments are not called max/min
* Continue without any check and throw unexpected values.

I repeat, is my opinion and maybe I wrong. Anyway I prefer the first choice (throw an exception)
Could we add the Khronos copyright to this file?
@stof i had tried to use WebTestCase but it was giving a problem with not finding some autoload.php  for some reason i did not know. @l3l0 once told me how to solve this but i cannot recall.
`s/founded/found`.
The only reason I'll agree with you is because I had a chance to explain this in the SaveDocumentImplementationAction class. :) I'll reduce it to the first 2 sentences.
Playing with a protected attribute inside a closure is always a challenge. I liked the base "idea" to set a initialiser only for a desired meta without the need to provide an exhaustive closure, but maybe your solution is a better compromise since customizing metas is not a "daily need".
To provide protocols for the proxy. e.g. to proxy Minecraft, you need Vanilla.
This is really specifically testing a "GAE" context. Could have a file that either stubs/mocks the _insert_tasks so there's no GAE requirement and then have this file be used to test the GAE context.
in all other cases you return this[key].valueOf(), but in this case you return this.valueOf(). Is that a bug?
get() return a model instance and filter() returns QuerySet

In[11]: Neighborhood.objects.get(pk=689)
Out[11]: <Neighborhood: Germantown, Westside>

In [12]: Neighborhood.objects.filter(name="Germantown, Westside")
Out[12]: [<Neighborhood: Germantown, Westside>]

The call to n.name will fail if n is a QuerySet
Shouldn't this be an empty array instead of an array with empty string?
I'm curious about this, why do we need to check for nil?
pinagsama mo na lang ung tests no?
Something went wrong with encoding of Andreas' name here :)
@harjis if-else ja sit oma iffi ennakoille. Rakenne mielestäni väärä. Palautetaan ennakkokeissi ekaks niin kuin se oli enne ja sit elsessä JT-keissi.
Sorry I hadn't seen your last comment, and I've just merged your PR and removed the try/except clause.
Since `orderedFunctions` should only contain strings, please change the type annotation to `{Array.<string>}`
@retornam: I can, but I think we should address @m8ttyB's overall question on all of these pull requests, which is: Is it acceptable that if one link in a group fails then the rest are not tested?

I can change the way all of these work, so that they can be parameterized, but that will be a lot more code. In IRC today @davehunt suggested that he didn't feel we should parameterize all of these, but I think it is ultimately @retornam's call.  

When I first started down this road I was duplicating the logic already in some of the tests, which simply checked that a number of links were visible, and only had one assertion: either all were visible or the test failed. I guess from that I assumed that this approach is acceptable, but it would be good to get the final word from @retornam about this.
I think I get why you're bringing this stuff in, but I don't see it used anywhere.
Using prototype.setTimezone is also a better idea. There were a couple other instances where I was checking for that method. I don't see them changed above, was that your intention in this patch?
Same, `CoverageCraft()`
We shouldn't need the else here, since we just want it to do nothing if the work is unposted.
The second arg shouldn't be passed with a block though, right? The code you added will raise an exception with that example code.
Good point. I'll change that.


UI issues -- I haven't seen any problems but its only been a few days.

On 6/6/12 2:47 PM, "Erik Michaels-Ober"
<reply+p-940102-f628146e73d0dd52d584bd233fd5bfa6948488ff-57294@reply.github
.com> wrote:

>> @@ -6,7 +6,7 @@ Gem::Specification.new do |gem|
>>    gem.add_dependency 'bbenezech-nested_form', '~> 0.0.6'
>>    gem.add_dependency 'sass-rails', '~> 3.1'
>>    gem.add_dependency 'bootstrap-sass', ['~> 2.0', '>= 2.0.3']
>> -  gem.add_dependency 'jquery-ui-rails', '~> 0.2.2'
>> +  gem.add_dependency 'jquery-ui-rails', '>= 0.2.2'
>
>Any reason you're using `>= 0.2.2` instead of `~> 0.5`? I highly
>recommend reading [this blog
>post](http://yehudakatz.com/2010/08/21/using-considered-harmful-or-whats-w
>rong-with/).
>
>Also, have you verified that this upgrade does not cause any UI issues?
>
>---
>Reply to this email directly or view it on GitHub:
>https://github.com/sferik/rails_admin/pull/1193/files#r940102
cant we reuse the partial from the arch / show ?
Creating a list with lots of vertices is pretty expensive and for removals a second traversal is required.

Do you think any of these approaches might work?
* Use a foreach with a function that glues the 'shouldRemove' check and 'processRemoveVertex' together (might require changes to the iterator/collection used inside Storage).
* Move the vertex and edge counts and the respective removal counts behind the Storage interface, so all parts of the removal can be done inside the Storage implementation.
How exactly does setting innerHTML on a parent DOM object cause a JS heap object to have a reference nulled out?? That's insane. If that actually is the case we need better documentation of what is happening in the source so you don't have a WTF double take in the future... or at least don't have a WTF double take that causes the code to be removed when it's still needed.
Lets move this class into the helpers/collection file. This is the only place that it is used and it seems as if this could be included conditionally if the user desired.
I think this is not needed, it is not returned or anything, removing those 2 lines should be fine, or not?
if (channelParameters.isDefined) ch.basicQos(channelParameters.get.prefetchSize))

Shorter and doesn't create an anon inner class :-)
Needs indenting
I would trim this group of hints -> I think the first and third sentences are repeating definitions laid out above.
you need to add the $fID = 0 otherwise if someone calls it without an argument then it will still error,
This isn't really a license.
Shouldn't these reference the current user_name?
I don't think HttpContext makes sense in Configuration. Context is created per-request. Configuration is meant to be initialized once at app start time. Perhaps our use of context this will not make a difference, but it would be easy to make a mistake in the future and end up using the wrong HttpContext object (eg: using the HttpContext.Current.Request object from app_start in a future request would be wrong).
@kellyredding - typo: unitl -> until
OK, I agree with that.  Would you be OK with handling that as a new card / pull req or do you need it in order to help evaluate this one?  Also, while I totally agree that it needs more docs, I wouldn't mind discussing some options on how we version that sort of thing, because I am a little leery of introducing Yet Another Independently Versioned Thing into the codebase if there is any way to avoid it.
When unexpected errors occur we've been using:
```javascript
callback({'code': 500, 'msg': err});
```
Does this work? Shouldn't it be Constants.ROLE_MEMBER ?
This looks weird. Maybe `jdiMirror` is a better name for the underlying field? Or even `underlying`.
whitespace after } needed
Typo... `th` should be `the`
Нет проверки на совпадение значения гена.
I will take out the $ for all three files.
The full clause is:

```
            index = html.indexOf('data-l10n');
            if (index == -1) {
              return -1;
            } else {
              return html.lastIndexOf('<', index);
            }
```

(see: https://github.com/highfellow/webclient/blob/52c27ab71908163b7a8160001d9a1aa7433c2237/js/vendor/l10n-browser.js#L59)

I.e. it's looking for a data-l10n attribute. If none is found then there's no next tag (return -1); otherwise skip back to the start of the tag and return its index. I'm using string.indexOf, not string.search(regex), because it's supposed to be faster.
this is a good place for pytest.raises.  Here's an example where I used this elsewhere:
    with pytest.raises(FieldError) as e:
        perftest_refdata.get_ref_data(ptm.project, "not a valid table name")

    exp = ("FieldError: Not a supported ref_data table.  Must be in: "
        "['tests', 'pages', 'products', 'operating_systems', "
        "'options', 'machines']")

    assert e.exconly() == exp

I would recommend using this all over these tests where you expect exceptions.
Ah yes, totally right
You'll want to load this from the environment, rather than having this hardcoded. Check out, for example, how this script [loads it from the environment](https://github.com/github/hubot-scripts/blob/master/src/scripts/iced-coffee-weather.coffee#L25) and then [checks that it is set](https://github.com/github/hubot-scripts/blob/master/src/scripts/iced-coffee-weather.coffee#L29-31)
Description says it all, so command doc being blank here might be ok?
This is wrong. You should be using `args[0]` instead of putting `request` in here (as it's always the first argument to Django views). Does this even pass tests? I'm not sure how this will behave with multiple `args` and `kwargs` as a decorator.
Yes - I was thinking of an optional parameter (possibly matching the GIT_REMOTE_DOWNLOAD_TAGS enum from libgit2). I will go ahead and include this.
why not importing as usual from numpy.testing?
I think it's dangerous to use misc type. misc is used for other things such as continuous HPI on neuromag systems.

I would allow to pass picks and say that if picks is None then misc channels are used.

but it will break if you keep the original misc channels
We can add the scaling like you did it, but we have to mention that we scale using the assumption that all vertices in the label have the same time course. Alternatively, could we scale such that the power of the label time course is the same as the average per-vertex time course power inside the label?
why would positioned start after the at here?
Needs i18n.
I don't think xis is ever closed?
А головой кто думать будет, Игно?
Эээ...а тут разве не так?
sorry that we use dom as :

```haml
.pic
  .medium
    = image_tag ....
```
Me parece que la idea original era que el `el` sea lo que para vos es el `uploadButton`. Me parece que se está haciendo trabajo de más para no cambiar la forma en que usamos el `UploaderElement`. De cualquier manera me parece que es una solución práctica ya que espero que en un futuro nos quitemos de encima nervoustissue-uploader y usemos una implementación más limpia.
what's 127.0.0.2?  is there something we need to do to support this on the server?
do we have reason to think that this is relatively efficient? I'm pretty sure string(char) calls print_to_string, and best-case appending characters to strings like this generates rope string. I suspect we'd be better off using indexes to keep track of items and only copying once, at the end.
a class doesn't create an object, but you can create an object from a class using the classes initialize method (Class.new). Each object has a class, which defines the type of thing it is - so what it can do (methods) and have (state).
this should be refactored in the main framework file ; add a method to do XMLHTTP requests
Same here... Is it normal that it is commented ?
Yes, this is _the_ tricky part.  Depending on the behavior we want to have we can consider 2 objects o1 and o2 being equal if:
* `o1===o2`  - this is reference equality
* `angular.equals(o1, o2)` - this is "deep" equality
* other methods (`angular.toJson(o1)===angular.toJson(o2)` for example - but this is not reliable at all)

I've started implementing equality based on `angular.equals` - this will work fine for all object types but want be super-super fast - O(n*n) - unless anyone knows a good hash-generation function for objects.
True. I guess this would be more of a developer/API cautionary exception, to better monitor your transaction code.  

Probably doesn't hurt, just wondering about the semantics  

--  
Matt Behrens


On Tuesday, December 4, 2012 at 11:39 PM, Valentin Alexeev wrote:

> In libgreader/googlereader.py:
> > @@ -177,10 +179,35 @@ def removeItemTag(self, item, tag): > return self.httpPost(ReaderUrl.EDIT_TAG_URL, > {'i': item.id, 'r': tag, 'ac': 'edit-tags', }) > > + def beginAddItemTagTransaction(self): > + if self.inItemTagTransaction: > + raise "Already in addItemTag transaction"  
> It depends. In this implementation starting a new transaction without commit will effectively wipe out the backlog without applying it. In theory we can commit here as there is no way to rollback now yet it's not the normal semantics.
>  
> —
> Reply to this email directly or view it on GitHub (https://github.com/askedrelic/libgreader/pull/17/files#r2317497).  
>  
>  
>
From the JLS: "It is permitted, but strongly discouraged as a matter of style, to redundantly specify the public modifier for interface methods." :)

Interface methods are already public, without the need of the modifier.
Nah, this is good. We prefer not to rebase published branches.
My feeling is that "What's new" is for high-level user-visible changes (sort of like an advertisment for the new release), and CHANGELOG (which I believe we are still using) is for smaller details that users need to know about.  We also have api_changes for things where we know users need to change their code.  (I don't think this falls into that category because `subplots()` would have raised an exception before and now does something new).
This will cause `getMatchingFilterList` to rematch all the filters every time it is called if there are no matching filters. The `isset` function cal was intended to handle that case
try use .delegate() instead off().on()
That's cool. Good to know.
Awesome! Thanks for cleaning this up.
We shouldn't have s3d classes anymore. I'll go ahead and change them to oae-
good morning

oh, yeah, sorry, I should have added the question to the pull request: what was the intention of having Cancel written into the success message field in the first place?
This is displayed after finishing the Upload, that means that there is a Edit, Remove and Delete button.
And without this being removed, it has a green status text "Cancel", which is just there, not clickable, nothing.
And once you click Edit, Cancel gets replaced by "Editing ..." anyway, and after that "No changes".

So I removed it.
I left the html there, since its filled with text via javascript later, and we might decide one day that we want to write "Upload Successful" or something into it.
CSS generated content like `:before` doesn't work in IE7. If this means it only shows the labels, that's acceptable - I'm assuming you're testing in IE7 as well, right? :)
Hmmm, that was a pre existing function which I just shifted over when i was moving all the CSV download stuff to the helper. You're right, it doesn't make sense. Getting rid of it. :)
what happens if the experiment is enabled on page load, but someone clicks 'disable' in another browser?  this browser will still say 'disable', but since it just calls `toggle_enabled`, it will actually re-enable it, which can be very confusing behavior.  these kinds of race conditions can and do happen if multiple people think they have exclusive control over a cpanel.  it's best to have idempotent actions with immediate status messages reflecting the outcome.

instead of 'toggling' from this button, it might make more sense to have separate actions, 'enable' and 'disable'.
use `tap` or `Hash[]` here:

```ruby
{}.tap do |hash|
  metric_counts.each do |metric_count|
    hash[metric_count.metric] = metric_count.count
  end
end

# or:

Hash[metric_counts.map {|metric_count| [metric_count.metric, metric_count.count]}]
```

see
http://www.ruby-doc.org/core-1.9.3/Hash.html#method-c-5B-5D
http://ruby-doc.org/core-1.9.3/Object.html#method-i-tap
Can't put closures in main config... You need to move this to the service config (probably)
Use [`_.has`][1] here instead. :)

[1]: http://underscorejs.org/#has
yes yes, also, in Java you get 100 column lines so there is no need to be frugal :-)
Haha, didn't even realize it :)
Getting Started _in_ Windows.
please remove this and add the license header instead
я думаю это должно лежать в конфигах
Вроде как в этом уже нет необходимости после подключения работы с БД
This backfires if `(pred nil) ;=> true`. Easy fix:
~~~clojure
(pred (first (filter pred a-seq)))
~~~
This cannot be correct: $width is 0, so this will divide by 0.
IMO this should be isAuthenticated() and the one above should be authenticate()
It'd be better style to use the point constructors here:
ofPoint point6(500,400,0)
No. Args is a way to set a hash of arguments to give to a command. This is a class specifying an argument of a modular input.
Public fields are not recommended by the Java community.
Some unnecessary space?
only add help = '' and this message will be display if we call like manage.py help updatehoroscope or. manage.py updatehoroscope --help
Import only modules. Not its content. See how we are doing it elsewhere. (The only exception is `urls.py`.)
Uh is this code ugly. :-)
Should you use `var` in for loop or not? What is best practice? Please use it everywhere if yes.

For iterating over lists and dicts you can use jQuery's `each` method.
PATCH is still to broad. It means you can update any field in the object. We probably do not want to allow this? You should define all other fields as readonly.
Aren't only comments with empty description loaded as "COMMENT"? If so, it seems that adding comment will convert it to "COMMENT:COMMENT" in the next read.
if config.maxQueueAge isn't defined, "get" will throw an exception. you can omit this line entirely and just start with #154 below...
Yeah, I know. I wish they would have spent more time designing that...
ga, ya, was resolving a merge conflict. removed.
@cyberbrest: TARGET_UNIT_TARGET_PASSENGER is definitely not TARGET_REFERENCE_TYPE_TARGET type of target because it sends Target GUID: 0x0 <- targets of that type have nonzero val there, so TARGET_REFERENCE_TYPE_CASTER is ok there.
About other research you've made: no idea i don't have any tools anymore to check anything so listen to what Shauren says :)
To make this patch complete please remove "_TARGET" from the target type name and please remove TARGET_FLAG_PASSENGER from spell target flags - it's no longer needed since there're no explicit passenger target types anymore. I'm no longer a dev here anymore so I won't be much help but i'm sure you can get some help from other devs on irc.
Does the additions above need to be refactored? The code builds @deployables, but it is then not used in the response which I find confusing.
this shoudl be a nav element if you are following semsantic structure. Also, cannot have LI's with a wrapping UL or OL
With the `Hyperclient` refactor this will change.
Unsure about this one. Shouldn't the connection live in DoctrineORMModule?
You're not checking what is actually bound there.
Good question. Maybe ping @traviscrawford and @rangadi to see what they think?
could do something similar here:

```ruby
return "Heirloom #{@name} not found in catalog." unless name_exists?
```
Have you tested this? Because I am very sure that those parenthesis are wrong.

Also, you do not have to define a variable if you will just use it once.
This is fine, but for future reference you don't have to increment the version number because tito does this for us in the build scripts.
Guess it's `type_code` too then. Fixing.
I think it's better to retab the file file after changing the tab style!
I suggest using TryParse rather than relying on exception handling- seems a bit cleaner, but this is more of a style preference.
I'm pretty sure it's definitional, and I think that note is out of date. Spent a lot of pages of scrap paper on making sure on this.
Because it went over 79 columns.
ok, I see now, _cdf or _convolution below, different kernels
This was a quick and convenient way to grab all versions of a cookbook that had been uploaded. A flag probably makes sense.
At Twitter, it's standard to use the `do ... end` syntax instead of curly braces for multi-line blocks.  Also, this is another great place to use File.join:

```
IO.readlines(File.join(data_path, "UnicodeData.txt")).map do |line|
  ...
end
```
Yes, since as I mentioned on scrum yesterday, I tried fixes "here and there" and ended up with a disaster. This change was made by me only to get better failure message (a bit, like "size is not 1, but still, what is the size?").

This change did not serve any other purpose. But will fix it.
lol.  i guess.
Given the asynchronous nature of the client/server interaction, it would be worth considering whether `greetingsPanel.showStack(0)` might be better moved to where `greetingsScrollPanel.scrollToTop()` is.   By doing the showStack when the response comes back from the server, rather than when the request is sent, the display will make more sense during the interval in between.
Shame we have to support the backwards compatible nested lists, as then can't get the generics in the type signature right (I presume it should always be Iterable<String> and won't accept Iterable<LocationDefinition>). Good that we're deprecating it.
Perhaps given it's defined as config and attribute-sensor, we should return the attribute value? That is the definitive runtime value, rather than the "suggested config". For example, port config could be a range whereas the attribute is the actual port.

in this case, presumably they will always be identical.
So not sure about this comment. Nice to be consistent though....
because we have a closure here we don't need isRunningCallable at all I think.  or have the closure / isRunningCallable above and don't put any closure here.  (it defaults to { it } where it is the result of the function on the serviceUpAdapter.)
Add a line or two more here to make it obvious that it doesn't matter which property is `nil`.
Ideally, I would make a `debian_alternative` definition for that.
Comment #1: This pull request is for last week's code base. All references to "addthis" have been removed in the newer code base which will be sent in the next pull request.
uplaod -> upload