Allow user and other client properties to be set after starting crash service

Overview
Properties on SentryClient which are set after calling startCrashHandler are not sent along with crash reports. This includes user as well as tags. I have not verified this for extra.
It makes sense to begin crash handling as soon as possible in the app lifecycle, but at that point user information or other tag data may not be available, for example if the user has not logged in yet, or if user info is not kept in cache and is instead downloaded from the server on every app session. However, setting the user or other properties after starting crash handling has no apparent effect.
Expected Result
Setting user, tags, etc on the shared SentryClient instance at any point during the app's lifecycle, including after already starting crash handling, should cause that information to be sent with crash reports for any subsequent crashes.
Actual Result
No custom client data is sent with crash reports if set after starting crash handling.
Example
This works correctly:
SentryClient *client = [[SentryClient alloc] initWithDsnString:@"<DSN>"];
[SentryClient setShared:client];
SentryUser *sentryUser = [[SentryUser alloc] initWithId:@"1"
                                                  email<EMAIL_ADDRESS>                                               username:@"Claudia Thompson"
                                                  extra:@{}];
[[SentryClient shared] setUser:sentryUser];
[[SentryClient shared] startCrashHandler];

This does not work correctly:
SentryClient *client = [[SentryClient alloc] initWithDsnString:@"<DSN>"];
[SentryClient setShared:client];
[[SentryClient shared] startCrashHandler];

// Later
SentryUser *sentryUser = [[SentryUser alloc] initWithId:@"1"
                                                  email<EMAIL_ADDRESS>                                               username:@"Claudia Thompson"
                                                  extra:@{}];
[[SentryClient shared] setUser:sentryUser];

Looking through the source, it seems like user info isn't persisted to disk anywhere in either Sentry or KSCrash. Setting user on SentryClient after crash reporting is started results in the user info getting set on crashHandler since it will not be nil at that point.
public var user: User? = nil {
    didSet { crashHandler?.user = user }
}

However, by then any enqueued crash reports will have already been submitted, since that happens immediately when calling startCrashHandler.

// SentryClient.swift

internal(set) var crashHandler: CrashHandler? {
    didSet {
        crashHandler?.startCrashReporting()
        crashHandler?.releaseVersion = releaseVersion
        crashHandler?.tags = tags
        crashHandler?.extra = extra
        crashHandler?.user = user
    }
}

// KSCrashHandler.swift

internal func startCrashReporting() {
    // Sychrnoizes this function
    objc_sync_enter(lock)
    defer { objc_sync_exit(lock) }

    // Return out if already installed
    if isInstalled { return }
    isInstalled = true

    // Install
    installation.install()

    KSCrash.sharedInstance().searchThreadNames = true

    Log.Debug.log("Started Sentry Client \(SentryClient.versionString)")

    sendAllReports()
}

I'm actually confused as to how the user info even gets through if set before calling startCrashHandler. In the didSet closure for crashHandler, the reports are sent before the user info is updated. Assuming the reports are formatted and sent asynchronously, wouldn't that at best be a race condition with updating the user info for the report format?
In other words, shouldn't crashHandler?.startCrashReporting() be the last line of didSet?

@ndonald2
Hey thanks for that detailed bug description ... I am currently looking into that.

@HazAT
Thanks for looking into it and writing the test. Unfortunately the test does not represent the scenario I was describing.
In the event of an actual crash, all application state and memory contents will be destroyed. So even if the user had been configured during the application's lifetime, it will need to be reconfigured on the next application launch. However, as soon as startCrashHandler is called, any pending crash reports will be formatted and uploaded. So if the user has not yet been configured, the report will not be associated with a user.
Here's an outline of the timing which I hope illustrates the problem:

App Launch
Start Crash Handling
Set User on SentryClient
App Crashes
App Opened Again
Start Crash Handling

Pending Crash Report Sent (No User Data Set)

Set User on SentryClient (Too Late)

There is nothing in the documentation that states that you must set user prior to calling startCrashHandling in order for crash reports to properly be associated with a user, so I would expect it to work either way. Granted, many (if not most) apps will already have the necessary information at hand to setup the user when the application first launches, i.e. if the user information/object is persisted to disk, but this is not the case for all apps, including mine - the user's profile is retrieved from the API on every app launch.
Because reports are formatted and sent immediately on the next app launch when calling startCrashHandling, it seems that the only way to fix the issue as described would be for SentrySwift to persist user (and tags, etc) to disk and then load that data when SentryClient is reinitialized on the next app launch.
Please let me know if that does not make sense. If this is not a use case that SentrySwift will support, then I would request that the documentation be updated in order to indicate that user must be set prior to calling startCrashHandling in order to have a user associated with a crash report. Thanks!

@ndonald2
Thanks for the detailed description, now I fully understand the problem.
I've recently discovered that behaviour when using breadcrumbs ... and yes it should work that way that in case of a crash, a snapshot of all set properties should be persisted and stored and sent with these properties on the next startup.
I am gonna definitely fix that, stay tuned.

The new release 1.4.3 fixes this issue, thanks again for pointing that out.

Great, thank you!


Typing resolutions

It sounds like the resolution problems tackled in https://github.com/typings/typings/issues/123 are critical to this plugin's type checking feature. I'd be really interested to see what can be done to make this work, if you don't mind helping me understand this area a little better.
Firstly, jspm-npm will handle conversion of npm packages converting the Node resolution algorithm into the SystemJS resolution algorithm.
Are you saying that typings themselves are written assuming the Node resolution algorithm? Which assumptions are these usually based on? For example, are extensions usually used? Do directory requires or json requires get used? How exactly does SystemJS's naive import not work with this resolution currently?

A little bit of background on TypeScript typings - originally .d.ts files were included using /// reference path="./typings/angular.d.ts" comments in the source files which needed them. These declaration files contain external module declarations like this:
declare module "angular" {
   function bootstrap(modules: Module[]);
}

When you do import angular from "angular" TypeScript then finds the "angular' module declaration and uses that for type-checking. This is quite straightforward for the plugin as it knows all the files which it needs to load to type-check a source file.
Recently there has been change so that you can deliver typings files, these are a little different to the one above because the file itself is the module, so the one above would look like:
export function bootstrap(modules: Module[]);

and when you do import angular from "angular" TypeScript will see that the "angular" package has a typings file and use that when type-checking the file. So how does it know whether the angular package has a typings file? Well the algorithm goes something like:
a) Look in package.json for "typings" field. If found use that.
b) Resolve the imported .js file using node.js resolution and see if there is a .d.ts file of the same name
Rule (b) also applies when doing import router from "angular/router". The exact algorithm is described here
So this is the difficulty when loading tin the browser, because the plugin doesn't know which packages have typings files. SystemJS resolution can't be used because that needs to resolve to the .js file.
Angular2 delivers multiple typings files in the npm package and this makes it impossible to type-check in the browser unless you switch back to using one of the old-style declaration files. Solving the angular2 problem ought to be the priority here I think.
If jspm could record any .d.ts files it finds when installing packages so that they were available somewhere in the runtime config then that might work, could jspm-npm do that? Otherwise the typings project may be able to provide this information but its not quite clear how that is going to pan out right now.
I hope this is clearer and we can find a way forward.

bit more info for background - there are two types of typings files now - "ambient" and "external" - angular2 uses the externals (they are basically es6 modules, more or less) and effectively its a reasonable guarantee that there's 1 .d.ts file per .js file - this is how typescript outputs them.
its unlikely we'll switch back to the old style (ambient) any time soon / ever, and ideally we'd like to not have to tell consumers of NG2 to do the same.
the typings project is for ambient modules only (afaik) and thus we won't be delivering the typings to it's registry for angular2.
@guybedford @frankwallis happy to have a pow-wow if you guys want to work through this.

see also https://github.com/Microsoft/TypeScript/issues/6012 - and as of TS 1.8 there's a new (configurable) resolution strategy which is ~mostly JSPM style.

@robwormald yes maybe I am using wrong terminology so the first example above is "ambient" and the second one "external".
In the angular2 case, then, a simple boolean flag to say the package does have external typings would be enough. The plugin would always resolve and fetch external typings for files inside that package. That flag could go in:

angular2 package.json typings: true (probably illegal)
jspm section of angular2 package.json
jspm registry
typescriptOptions of consumers
package config of consumers

So to account for most common scenarios, jspm could retrieve the value of typings from package,json and make it available to the plugin in runtime configuration. This field (or some other flag) would also be settable in the jspm registry and setting it to true would mean resolve external typings for all modules. Does this sound possible?

@robwormald I don't think I am able to use the new algorithm in the browser, because it needs to search for files. It solves the issue of missing intellisense and being able to use tsc with jspm and external typings though, which is progress.

@frankwallis let me see what we can do about your first question, we should be able to make one of those options work.
The second though - assuming you know that there are typings, no search should be needed, as they are 1 to 1 for the actual source files. (that is, foo.ts becomes foo.js and foo.d.ts on npm) - or am i misunderstanding you?

@robwormald thanks, I'm really keen to get this working.
I need to have a look at the how this new paths compiler option is implemented and how it interacts with moduleResolution: node, but yes I suppose my point is more about implementing moduleResolution: node without doing nodejs file-system calls.

@robwormald my understanding is that typings is for external modules. See here and here. So then Angular2 could be added to the registry. Or have I missed something?

@frankwallis @robwormald thanks for the clear descriptions here. It does sound like Angular would be the nice case to crack here to set the precedent.
@robwormald did you say we really can assume the convention then that the typing file for a fully-resolved .../full/path/to/module.js is located at .../full/path/to/module.d.ts? Or would such a convention have edge case limitations when scaling over npm?

@ximenean as I understand it the aim of the typings project is to allow ambient modules to co-exist with external modules until everything has been moved to external modules. It's all rather confusing though...
Some new docs have just arrived on the subject: https://github.com/typings/typings/blob/master/docs/how-typings-makes-external-modules-first-class.md

@guybedford that is only true for the angular2 package.
As an example of another sort of package angular/zone.js contains a typings field in package.json (https://github.com/angular/zone.js/blob/master/package.json#L6)
{
  "name": "zone.js",
  "version": "0.5.13",
  "description": "Zones for JavaScript",
  "main": "build/lib/zone.js",
  "typings": "build/lib/core.d.ts",

This file will contain bundled typings for the whole package.

@frankwallis as far as I can tell zone.js does use the same convention within in its file structure. It does seem a very sensible convention that would be easily compatible with SystemJS if this is the way things are moving.

@guybedford yes and in fact the core.d.ts typings entry in package.json looks  incorrect to me, it should point to zone.d.ts. Another example is RxJS, which in 5.0.0-beta also follows the same convention.
I think it is quite likely that packages which are developed in TypeScript will follow this convention, but packages which are developed in JavaScript may choose to have a single bundled typings file.

I have managed to get the angular2 example type-checking by telling the plugin which packages have typings files, the configuration looks like this currently:
typingsMap: {
   angular2: true,
   rxjs: true
}

This works because all imports from these packages specify the module name to be imported (ie import {bootstrap} from "angular2/boostrap". I don't know how to find out the main of a package within SystemJS, so import {bootstrap} from "angular2" would not currently work.

In jspm 0.17, the main entry point problem here goes away because the main is resolved as part of the normalize step instead of fetched through a pointer module. In cases where you don't have a true value for the typingsMap, perhaps you can just infer from the fact that the plugin fetch hook is being called for a source and load the typing file in parallel at that point (even if it is a 404)?

@guybedford that's good news about the main entry point issue in 0.17, and I was hoping you would say that! Unfortunately I don't think the plugin fetch hook will always be called in this situation, as the actual file will resolve to a .js file, but TBH I don't think it's a big deal if it can't be fully supported until 0.17, as long as angular2 works.
What are your thoughts about putting this configuration into the package config? I am happy to do a PR if you point me in the right direction.
I would propose that:

The typings field would be loaded package.json when availble
It could be overridden in jspm registry, where it can also be set to true
It could be overridden in user package config or overrides, where it can also be set to true

Does this sound ok?

@frankwallis I'd strongly suggest moving on the 0.17 upgrade path and encouraging users here to as well. I would have published it as stable already, but am going slowly on the stable branch rather to give users time, but individual projects should push forward.
The only way to handle a custom field through the package configuration is through meta exactly as in https://github.com/frankwallis/plugin-typescript/issues/96 -
{
  "meta": {
    "*.ts": {
      "loader": "typescript",
      "typescriptOptions": {
        "typings": {...}
      }
    }
  }
}

Would you still consider that if a TypeScript file is run through the fetch hook, the loader would request the typing by default? If the fetch hook is not run for that specific file (eg for 0.16 mains), then a field like this makes sense certainly. It could potentially also act as an opt-out for the automatic typing loading if that is implemented.
My biggest concern is what normalization space this field is acting in? Multiple plain names can refer to the same module, so you'd need to run that name through System.normalizeSync or System.decanonicalize to get into URL space for name comparison (the only space in which comparison is unique). normalizeSync probably gives the most naming flexibility handling all the many-to-one options.

The other thing is even if using normalizeSync, it should probably be a contextual normalization as well to get all the contextual package maps. It's not completely clear though which contextual parent is intended for such an unnormalized typing (with System.normalizeSync(typingName, whichParentName?))

If it's a typescript file we don't need any typings, the typings files are only needed for packages delivered as .js files.
The plugin needs to know:
a) the value of the typings field in package.json if there is one
b) a flag to tell it that a package provides typings files alongside all of its js files
So the typings field in package config would either be a string for situation (a) or true for situation (b). The plugin handles all the normalization stuff already I think.
I have put in the typingsMap field into typescriptOptions so that I could see if this was going to work, but it is really meant as a stopgap until this configuration can come from the packages themselves.

Ahh, right, thanks for bearing with - sorry it's my distance from these workflows showing in these simple errors. I'm really so grateful you are working all of this out!
So the '.js' file has matching '.d.ts' typings usually when it was compiled from TypeScript as separate files then? And thats what Angular and Rxjs do? Is there any indication in the source code of those .js files that these typings files exist? You mentioned the comment method previously?
Would a TypeScript package ever have a typings field for its dependencies? Or is the point here that we associate all typings through overrides to the underlying files?
So in jspm 0.17 "meta" can be provided as a field in the package.json (along with all other SystemJS package config options). The code example in my previous comment could be the package.json file itself or the override.
Thinking further, could you not just do something like:
{
  "meta": {
    "*.js": {
      typings: true
    }
  }
}

So that we use the individual metadata per-module to indicate which files have associated typings?

might be better with "external" vs "ambient" to denote parallel typings (external)?

hey no problem, I just want to make sure we get to the right place.
So, yes, angular2 and rxjs provide .d.ts files alongside their .js files, but there is no way to figure this out without looking at the files which are in the package.
Some other packages can populate a typings field in package.json which will point to a single .d.ts fie which corresponds to their main file. It remains to be seen how much this will be used.
I'm not quite clear on your question, but I think the answer is that a TypeScript package would not have typings for it's dependencies - the plugin should automatically locate those typings in the dependency package if they exist, and feed them into the compiler. This is why it is important for the plugin to know what typings are present.
Providing this information in the meta might work, if I can access it at the right time. As an example:
somefile.ts:
import {boostrap} from "angular/bootstrap"
...

At this point I normalize "angular/bootstrap" -> "http://jspm_packages/.../bootstrap.js" and I then need to get the meta for that file. I'm not quite sure how to do that?
I will have a play around with this and report back. If it works then I think we will have a solution as this meta can be added into the jspm registry presumably, or put in the angular2 package.json file?

There's a hack that can be used to get the metadata for a package, but it won't work in future versions when the locate hook is deprecated. We can make sure the use case sticks around somehow though.
Basically you can do:
var metadata = {};
System.locate({ name: 'http://jspm_packages/.../bootstrap.js', metadata: metadata })
.then(function(address) {
  // metadata.typings, metadata.typescriptOptions etc should all now be populated correctly,
  // respecting the composition of all global metadata, wildcard metadata and package metadata correctly
});

Yes meta can be in the original package.json or the jspm registry. I hate to be introducing "another" way of doing this stuff, but we do just need to let these metadata conventions all fight it out in the wild for a bit I think.

ok I'll give that a try and report back. I also think that this stuff is likely to change on the typescript side (see https://github.com/Microsoft/TypeScript/issues/6950 for example), so I agree it's good not to be over committed at this time.

@robwormald I don't understand - can you clarify?

I have added support for specifying typings configuration in metadata and it is released in 4.0.0. The angular2 project now type-checks as I have added packages configuration here:
    "angular2": {
      "meta": {
        "*.js": {
          "typings": true
        }
      }
    },
    "rxjs": {
      "meta": {
        "*.js": {
          "typings": true
        }
      }
    }

Would it be possible to add this meta information to the packages overrides in the jspm registry? So that the consumer of the library doesn't have to set this in his jspm.config.js.

@frederikschubert yes that is the plan, I just need to get it working with a manual override first to verify.

Awesome! Please keep me posted if you hit any further issues with this approach.

@guybedford thanks, I now want to add the packages configuration above into the jspm registry. It is using wildcards as metadata keys to match all the js files in the package - is it ok to use wildcards for metadata in the registry because I don't think they work for overrides?

@frankwallis yes this is perfectly fine and the way things are going. A "meta" property in overrides and the registry is supported, but only in jspm 0.17.

@frankwallis I just tried to use the config you have in the examples for angular2. I am using reflect-metadata and when I use the following config:
SystemJS.config({
	transpiler: "ts",
	typescriptOptions: {
		"typeCheck": true,
		"tsconfig": true
	},
	packages: {
		"src": {
			"defaultExtension": "ts",
			"meta": {
				"*.ts": {
					"loader": "ts"
				}
			}
		},
<EMAIL_ADDRESS>{
			"meta": {
				"*.js": {
					"typings": true
				}
			}
		},
<EMAIL_ADDRESS>{
			"meta": {
				"*.js": {
					"typings": true
				}
			}
		}
	}
});

It fails to load Reflect.d.ts. I am using karma-jspm and this plugin for running tests on .ts files and I get the following:
Chrome 49.0.2623 (Mac OS X 10.11.3) ERROR: 'TypeScript', Error{originalErr: Error{}}
Chrome 49.0.2623 (Mac OS X 10.11.3) ERROR: 'Potentially unhandled rejection [4] Error: XHR error (404 Not Found) loading<EMAIL_ADDRESS>        Error loading http://localhost:9876/jspm_packages/npm/reflect-metadata<EMAIL_ADDRESS>
I am not sure why it tries to load that file, maybe because when import 'reflect-metadata'; it actually loads Reflect.js?
I can also see that there is a Reflect.ts which contains typing definitions, maybe it's a mistake on their behalf and should have been Reflect.d.ts?

By setting typings: true the plugin will try to load typings for all js files with the same name as the js file but with a .d.ts extension. To load typings from a different file, specify the filename of the typings file, so I think the config should be:
<EMAIL_ADDRESS>{
            "meta": {
                "Reflect.js": {
                    "typings": "typings.d.ts"
                }
            }
        },

That gives me:
Chrome 49.0.2623 (Mac OS X 10.11.3) ERROR: 'TypeScript', Error{originalErr: Error{}}
Chrome 49.0.2623 (Mac OS X 10.11.3) ERROR: 'Potentially unhandled rejection [4] Error: http://localhost:9876/jspm_packages/npm/reflect-metadata<EMAIL_ADDRESS>detected as esm but didn't execute.
            at tryCatchReject (http://localhost:9876/base/jspm_packages/system-polyfills.src.js?3aa57969dce4ecea4c51aab540f372d15cc886b6:1252:30)
            at runContinuation1 (http://localhost:9876/base/jspm_packages/system-polyfills.src.js?3aa57969dce4ecea4c51aab540f372d15cc886b6:1211:4)
            at Fulfilled.when (http://localhost:9876/base/jspm_packages/system-polyfills.src.js?3aa57969dce4ecea4c51aab540f372d15cc886b6:999:4)
            at Pending.run (http://localhost:9876/base/jspm_packages/system-polyfills.src.js?3aa57969dce4ecea4c51aab540f372d15cc886b6:890:13)
        Error loading http://localhost:9876/jspm_packages/npm/reflect-metadata<EMAIL_ADDRESS>
And the<EMAIL_ADDRESS>generated by JSPM looks like:
{
  "main": "Reflect.js",
  "map": {
    "crypto": "@empty"
  },
  "format": "cjs",
  "meta": {
    "*": {
      "globals": {
        "process": "process"
      }
    },
    "*.json": {
      "format": "json"
    },
    "typings.d.ts": {
      "format": "esm"
    }
  }
}

So maybe that is not the right config?

Yeah, so the right way for this is:
<EMAIL_ADDRESS>{
	"meta": {
		"Reflect.js": {
			"typings": "Reflect.ts"
		}
	}
}

Maybe this should be reported as Reflect.ts should probably be Reflect.d.ts.

What is the current status of this issue?
As far as I tested, typeCheck works on server side compilation, but not yet on client side.
At least when using external dependencies I get errors:
logger.js:17 TypeScript http://localhost:9987/jspm_packages/npm/@ngrx/store@1.5.0/interfaces.d.ts:1:28Logger.error @ logger.js:17(anonymous function) @ format-errors.js:13formatErrors @ format-errors.js:7(anonymous function) @ plugin.js:39
logger.js:17 TypeScript Cannot find module 'rxjs/Observable'.                 (TS2307)

Ambient dependencies seem not to cause problems.
I'm using lodash in the sources and the matching ambient typings are loaded.

I think this is working, see the angular2 example
What version of jspm are you using?

I'm using "jspm": "^0.17.0-beta.13".
Which version was used for the example? It's not listed in the package.json

It's the same version, it's listed in the root package.json.
If you can provide a repro I will take a look.

I have tried example with Angular2 RC and it won't work. Maybe I'm doing something wrong. Please could you update example to Angular2 RC?

Hi @frankwallis, sorry for the late response. I recentlypublished ng2-jspm-template. Here the problem ocurrs when you turn on typeCheck inside jspm.conf.js and trigger TypeScript compilation inside the browser (for example via hot-module-replacement or starting without development bundling).

@svi3c thanks for sharing your project.
The issue with Cannot find module 'rxjs/Observable' can be resolved by changing the packages configuration in jspm.config.js:
    "rxjs": {
      "meta": {
        "*.js": {
          "typings": true
        }
      }
    }

becomes
<EMAIL_ADDRESS>{
      "meta": {
        "*.js": {
          "typings": true
        }
      }
    }

It seems that when the packages configuration and map entry for a package happen in different ``System.configcalls then the metadata is not being found. In your case thetypings: true``` metadata is found for @ngrx but not found for rxjs or @angular/*. This could well be related to https://github.com/systemjs/systemjs/issues/1255
Really this configuration should be setup in the jspm registry but that has not happened yet. In the typescript example project I have moved this configuration into package.json which means that jspm writes it to the config file with the map configuration and makes it easier when updating versions.
The error with the hot reloader trying to reload files is happening because the files were originally coming from the bundle and then trying to be reloaded with the file:// extension. I think that plugin-typescript should not be writing the file:/// extension to the bundles, so I need to investigate that. The hot-reloading should work when not using the bundles configuration though?

@sokolovstas the angular2 example is now using rc1

Thanks for the clarification and support, @frankwallis! Yes, without using bundles, the hmr works in combination with typeCheck.

@frankwallis I'm having a similar (?) problem in CockroachDB. Two of our dependencies (redux and reselect) now ship with external typings, so we want plugin-typescript to load them rather than using typings to fetch these files. It doesn't work (no attempt is made to load the .d.ts files). I've tried the workaround you describe here to no avail - jspm install removes the overrides. All the work is here https://github.com/cockroachdb/cockroach/compare/master...tamird:ui-next-update
Any ideas?

I haven't tried it yet (it's late here) but on quick inspection the override should be:
<EMAIL_ADDRESS>{
        "meta": {
          "dist/redux.js": {
            "typings": "index.d.ts"
          }
        }
      },

Note the version does not have the ^ it is the actual installed version. typings: true means that all js files will have a corresponding .d.ts file with the same name (like angular2), for bundled typings like redux it should contain the path of the bundle.
Hopefully this will fix it, if not I will take another look.

@frankwallis updated, but that didn't fix it =/

Ok, that configuration only works in<EMAIL_ADDRESS>but in 0.16 you can achieve the same thing by adding global meta configuration in config.js:
  transpiler: "ts",
  typescriptOptions: {
    "typeCheck": "strict",
    "tsconfig": true,
    "targetLib": "ES5"
  },
  meta: {
    "reselect": {
      "typings": "src/reselect.d.ts"
    }
  },

Alternatively you can also install them using the typings tool apparently see this comment which would give intellisense support (I haven't actually tried this though...)

Indeed, both of the solutions you mention work, thanks.
However, why is it that this doesn't "Just Work" given that the information is already in reselect's package.json?

The plugin does not have access to the package.json, and even if it did there are some packages which contain typings and do not have a typings entry in packages.json. The algorithm TypeScript uses for finding external typings cannot be used from within a browser as it involves trawling the file system, so when external typings exist the plugin needs to know that they do exist and their exact location.
I agree that the configuration is painful, and I am open to better solutions but I just don't see any at the moment. Ideally this configuration will get added into the central jspm registry and then things will "Just Work" for those packages.

The above typings metadata can also be added with a meta override into the jspm registry for 0.17. Let me know if you'd like help with that at all.

@guybedford if this information was in the jspm registry, it'd end up in my generated config.js, correct?
If that's the case, why doesn't (or can't) jspm read package.json and populate the same thing? Seems to me that the jspm registry ought to be for overriding packages' own information as opposed to copies thereof.

@frankwallis, you mentioned that "the algorithm TypeScript uses for finding external typings cannot be used from within a browser as it involves trawling the file system". Would it be possible to support this for precompilation builds and only require the additional configuration for the browser?

@bmayen it would be possible, but I am reluctant to go down that route as it would mean the file resolution worked completely differently in the 2 scenarios and that would be a constant source of issues. It seems to me that if you are using that workflow then you may be better off type-checking with tsc and just transpiling with the plugin?

I have added a new compiler option typings to enable easier configuration of packages with external typings contained within. See here for more info.
Also plugin-typescript now supports the types option for when you have installed typings from the @types scoped packages. See here for more info.
I am going to close this issue now, if you find issues with either of these features then please raise them separately.


Use full absolute url in notifications

I just realized this error is due to missing imports and not to a problem in the code logic.

Painful experience than to scroll down the unittests github actions logs. Can you run the unittests locally @marcosValle and tell us where it fails? (I still could not reach the bottom on GHA). Thanks.

Painful experience than to scroll down the unittests github actions logs. Can you run the unittests locally @marcosValle and tell us where it fails? (I still could not reach the bottom on GHA). Thanks.
Something like docker-compose exec uwsgi /bin/bash -c 'python manage.py test dojo.unittests -v2'

It seems like others.tpl was not loading the necessary filters. All unit tests should be ok now.

There is also a 'download raw log' on the top right somewhere to view a raw txt file in your browser with the full logs.


Merging latest changes from master

OPS Build status updates of commit f4b94d3:
:clock10: Preparing: average preparing time is 46 sec(s)

OPS Build status updates of commit f4b94d3:
:clock10: Incremental building: average incremental building time is 17 sec(s)

OPS Build status updates of commit f4b94d3:
:white_check_mark: Validation status: passed

File
Status
Preview URL
Details

MicrosoftCollaboratePortal/connect-redirect.md
:white_check_mark:Succeeded

MicrosoftCollaboratePortal/intro-to-mscollaborate.md
:white_check_mark:Succeeded

For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.


Debug python script from eclipse (Ghidra <IP_ADDRESS>)

Hi,
I'm trying to use eclipse with GhidraDev plugin to debug a ghidra script in python.
The problem is that when selecting Debug- > Ghidra, it opens successfully ghidra, then I run my script but then there is no more actions. I can't use resume, suspend, step line etc. Is there some bug with latest version of Ghidra ? My version is <IP_ADDRESS>
Here is the logs I found inside eclipse:
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/opt/eclipse/plugins/org.python.pydev.core_<IP_ADDRESS>108191530/pysrc/pydevd.py", line 24, in <module>
    from _pydevd_bundle import pydevd_constants
  File "/opt/eclipse/plugins/org.python.pydev.core_<IP_ADDRESS>108191530/pysrc/_pydevd_bundle/pydevd_constants.py", line 365, in <module>
    from _pydev_imps._pydev_saved_modules import thread, threading
  File "/opt/eclipse/plugins/org.python.pydev.core_<IP_ADDRESS>108191530/pysrc/_pydev_imps/_pydev_saved_modules.py", line 97, in <module>
    import thread;    verify_shadowed.check(thread, ['start_new_thread', 'start_new', 'allocate_lock'])
  File "/opt/eclipse/plugins/org.python.pydev.core_<IP_ADDRESS>108191530/pysrc/_pydev_imps/_pydev_saved_modules.py", line 73, in check
    msg = self._generate_shadowed_import_message([module.__file__])
AttributeError: type object 'org.python.modules.thread.thread' has no attribute '__file__'
go_func.py> Finished!

yes ok thanks, but I think there is another problem is you read the log i wrote. Did you have an idea ? something wrong with pydev ?

Can you try using PyDev 8.3.0?  I see the 9.x versions recently came out and I haven't tried them yet....there might be a problem with them that we'll need to address.

problem is that I don't know if it is possible to downgrade. It's written pydev 8.3.0 inside the eclipse marketplace ...

Hmm, it looks like they didn't update the text in the Eclipse Marketplace...they are giving us 9.0.1 with it, despite it saying 8.3.0 in the description.  You can uninstall your current PyDev by doing Help -> About Eclipse IDE -> Installation Details -> Select PyDev -> Uninstall.
PyDev 8.3.0 can be manually downloaded here.
Instructions for installing the .zip file can be found here.

ok I will test with it

I am also having issues with PyDev 9, so we can use this issue to track the fix.  Once you confirm things are working with PyDev 8.3, we should rename this issue to indicate that it's a PyDev 9 compatibility issue.

latest version of eclipse do not work with your version, see screen

The latest version of Eclipse is 2021-06.  What JDK version are you using?

oups will update it tomorrow then. Im at openJDK-16

The original post in this issue is a pydev 9 problem. I get the same traceback.
Here is what I have found so far.
The problem originates in pysrc/_pydev_imps/_pydev_saved_modules.py which very recently changed significantly. Problem happens when pydev is checking for shadowed imports for the thread module. The check method is slightly before this in the file. The line that causes the crash is in the except handler for the check function, which indicates the main part of check isn't finding one of the attributes it expects on the thread module I believe. Those are ['start_new_thread', 'start_new', 'allocate_lock'].
This seems strange because the jython builtin thread module looks like it should have those functions. I can see why it wouldn't have __file__ attribute since it's a built in, but I don't think execution should even be in that except block.
I'll try to look around some more later.

In ghidra, thread.start_new does not exist. Tested in the python console.
import thread
thread.start_new_thread
<java function start_new_thread 0x2>
thread.allocate_lock
<java function allocate_lock 0x3>
thread.start_new
Traceback (most recent call last):
File "python", line 1, in
AttributeError: type object 'org.python.modules.thread.thread' has no attribute 'start_new'

start_new seems to be an old name for start_new_thread
I the jython tests that skips obsolete methods in the thread module.
There is also a note here indicating the obsolete aliases in thread module are not supported.

thread module
CPython's thread modules defines some aliases that Jython's doesn't. These aliases are considered obsolete and won't be supported by Jython.

I think the solution is some change over in the pydev. There is a file where they monkey patch start_new and even mentions of what look like a jython work arounds for something else in the same file.

I'm going to have to do more investigation.
There are still problems using pydev wtih ghidra jython beyond the start_new. If you fix that, the next error is this

Traceback (most recent call last):
File "", line 1, in
File "C:\Users\X\.p2\pool\plugins\org.python.pydev.core_<IP_ADDRESS>108191530\pysrc\pydevd.py", line 24, in
from _pydevd_bundle import pydevd_constants
File "C:\Users\Zechariah\.p2\pool\plugins\org.python.pydev.core_<IP_ADDRESS>108191530\pysrc_pydevd_bundle\pydevd_constants.py", line 365, in
from _pydev_imps._pydev_saved_modules import thread, threading
File "C:\Users\X\.p2\pool\plugins\org.python.pydev.core_<IP_ADDRESS>108191530\pysrc_pydev_imps_pydev_saved_modules.py", line 81, in
import threading;    verify_shadowed.check(threading, ['Thread', 'settrace', 'setprofile', 'Lock', 'RLock', 'current_thread'])
File "C:\Users\X\Programs\ghidra_10.0_PUBLIC\Ghidra\Features\Python\data\jython-2.7.2\Lib\threading.py", line 88, in
ThreadStates = {
AttributeError: type object 'java.lang.Thread' has no attribute 'State'

Maybe related to this jython issue with loading imports in multiple threads?
https://bugs.jython.org/issue2642
In the ghidra python console, java.lang.Thread.State also doesn't exist.

from java.lang import Thread
Thread
<type 'java.lang.Thread'>
Thread.State
Traceback (most recent call last):
File "python", line 1, in
AttributeError: type object 'java.lang.Thread' has no attribute 'State'

Strangely, it does show up in the dir listing. I know it's possible for a property to exist on an object and have its getter throw an AttributeError. Dunno what is going on.
print(dir(Thread))
['MAX_PRIORITY', 'MIN_PRIORITY', 'NORM_PRIORITY', 'State',  ....]
Any ideas?

Thanks for looking into this @zZeck.  I haven't had a chance to starting looking into this in detail but I appreciate your efforts.

What ever issue I was having with Thread.State must be unrelated. After relaunching ghidra today to look around more, it isn't happening.  Removing the 'start_new' check from pydev makes 9.0.1 work fine in ghidra.

I was able to reproduce all the same errors you are seeing.  I also did a test independent of Ghidra, with a standalone PyDev 9.1.0  project pointing to a full installation of jython 2.7.2, and got a similar error when trying to debug:
  File "/home/ryan/.eclipse/org.eclipse.platform_4.21.0_1473617060_linux_gtk_x86_64/plugins/org.python.pydev.core_<IP_ADDRESS>110031145/pysrc/_pydevd_bundle/pydevd_constants.py", line 365, in <module>
    from _pydev_imps._pydev_saved_modules import thread, threading
  File "/home/ryan/.eclipse/org.eclipse.platform_4.21.0_1473617060_linux_gtk_x86_64/plugins/org.python.pydev.core_<IP_ADDRESS>110031145/pysrc/_pydev_imps/_pydev_saved_modules.py", line 97, in <module>
    import thread;    verify_shadowed.check(thread, ['start_new_thread', 'start_new', 'allocate_lock'])
  File "/home/ryan/.eclipse/org.eclipse.platform_4.21.0_1473617060_linux_gtk_x86_64/plugins/org.python.pydev.core_<IP_ADDRESS>110031145/pysrc/_pydev_imps/_pydev_saved_modules.py", line 77, in check
    raise DebuggerInitializationError(msg)
_pydev_imps._pydev_saved_modules.DebuggerInitializationError: It was not possible to initialize the debugger due to a module name conflict.

i.e.: the module "thread" could not be imported because it is shadowed by:
b
Please rename this file/folder so that the original module from the standard library can be imported.

So, it doesn't look like there's much we can do on the Ghidra end of things right now, other than document that the latest version of PyDev we support is 8.3.0.

Thanks to @zZeck's efforts, this will be fixed in a future version of PyDev (likely the one after 9.1.0).  In the meantime, you can manually implement the change they did in their PR in your PyDev's plugins/org.python.pydev.core/pysrc/_pydev_imps/_pydev_saved_modules.py file.


Implementing pause/un-pause feature

Our team has a use case where we need to pause a task, and manually inspect the results of the tasks before allowing the workflow to continue. Therefore, the ability to pause and un-pause the workflow at any task would be quite useful.
If we implemented this, would Digdag be open to merging the pull request? If yes, do you have thoughts on how we should go about implementing the feature?

To be clear, you need to pause a task in your use case? or a workflow?
If you mean you need to pause a task, generally it's difficult to stop and cancel tasks in this kind of architecture. We can send a cancel request for a workflow running to the server but, tasks in the workflow may be sometimes completed before agents stop them. It's also not easy to design and implement how to pause operators and the factories, for example, how we can pause pg operator that already submit queries to the database.
If you mean a workflow, it's not easy to implement the feature but, we might be able to do that somehow. Personally it's OK for me to introduce such feature in Digdag if the implementation is good.
An ad-hoc idea to implement it is to change the table schema of tasks table, which manages the status of tasks running by agents. Because agents need to know that the task is now pausing or not. Then, the pausing task entries need to be kept on the table as completed tasks are deleted from the table. So, the pause feature will make the tasks table size large. the performance of tasks polling will decrease. The idea is not so good.

Hi @muga.  I'm on the same team as @muntac.
We were hoping for a pause feature for individual tasks that had not started to run yet.  We do not want to pause a task that is currently running.
For example, if you had a workflow with tasks:  A --> B --> C  --> D.
If we issued a pause command for task C then the workflow would be able to complete tasks A and B and stop.  The WorkflowExecutor would not start running task C or any of its descendents until an upause command was issued for task C.  When an unpause command is issued for task C then it would start running right away since all of its predecessor tasks have already been completed.  If task C has some predecessor task that had not yet completed then task C would wait until its predecessor finished (as normal).
If we issued a pause command for task C while task C was running then nothing would happen.  We understand that pausing the executing of a task that is currently running is very difficult to implement.  We do not need this feature.
We haven't looked to closely at the implementation of the tasks table yet.  We hoped that it would be as simple as introducing a new state for tasks and then modifying the WorfkflowExecutor appropriately.

Hello, @rszumlakowski and @muntac
What do you think to develop team_confirm_wait> operator like s3_wait>?
s3_wait> operator waits for the file to appear in Amazon S3.
Similarly, team_confirm_wait> operator wait confirmation completion.
If this operator complete, continues task_C and task_D will execute.
If this task will fail, the workflow stop before executes task_C.
+task_A:
  xxx>
+task_B:
  xxx>

+task_team_wait_until_confirm:
  team_confirm_wait>: condition...

+task_C:
  xxx>:

+task_D:
  xxx>:

You can develop original operator like digdag-plugin-example and df_wait>.
I have one caution, Maybe you need to change, executor.attempt_ttl and executor.task_ttl parameters in the current implementation.
Please see 0.9.6 release note.
This extension is an idea for future development.
Next step, the pause> operator like if> operator may be useful for wait over executor.attempt_ttl time.
It waits until: block completion.
+task_A:
  xxx>
+task_B:
  xxx>

+pause_tasks:
  pause>:
    until:
      +team_wait_until_confirm:
        team_confirm_wait>: condition...

+task_C:
  xxx>:

+task_D:
  xxx>:

And digdag unpause may be helpful for unpause task.

@rszumlakowski @muntac

We haven't looked to closely at the implementation of the tasks table yet. We hoped that it would be as simple as introducing a new state for tasks and then modifying the WorfkflowExecutor appropriately.

I think that you were taking a look around WorkflowExecutor#getTaskRequests. The getTaskRequests method is called by InProcessTaskServerApi#lockSharedAgentTasks. The method tries to access queue_tasks and queue_task_locks tables as you might know. We were almost on same page (sorry, table name was wrong. not tasks table).
The point is who manage the task status "pause". The natural way is that the status are stored on the tables I think.
@hiroyuki-sato
Thank you for suggesting! I think that you already know that "pause" functionality here is different from "wait". In my idea, "pause" is to skip pulling "pausing" tasks. And "wait" is to sleep appropriately. If "wait" is OK for @rszumlakowski and @muntac in their use case, your suggestion would be helpful.

Thank you for your responses @muga and @hiroyuki-sato.  We are interested in the "pause" functionality -- not the "wait" functionality.
We are going to look into a possible implementation for the "pause" feature.  After we have thought about it for a while we will get back to you.  If we implement it and if you are happy with it then we would submit a pull request sometimes in the next few weeks or months.

@rszumlakowski @muntac Okay got it. Thank you for suggesting. This feature is nice so, please feel free to submit a PR.

Close because of no activity.


Global appState plugin

Verified following scenarios

appState property as dynamic variable
appState property as reactive variable
appState property in computed property


RN 0.71.11 (cli) - Border radius with border bottom is broken

Description
The border bottom has a gap / white space if combined with the border radius.

React Native Version
0.71.11
Output of npx react-native info
System:
OS: Windows 10 10.0.22621
CPU: (12) x64 Intel(R) Core(TM) i5-10400F CPU @ 2.90GHz
Memory: 7.98 GB / 23.94 GB
Binaries:
Node: 18.16.0 - C:\Program Files\nodejs\node.EXE
Yarn: Not Found
npm: 9.5.1 - C:\Program Files\nodejs\npm.CMD
Watchman: Not Found
SDKs:
Android SDK: Not Found
Windows SDK: Not Found
IDEs:
Android Studio: AI-221.6008.13.2211.9619390
Visual Studio: Not Found
Languages:
Java: 11.0.18
npmPackages:
@react-native-community/cli: Not Found
react: 18.2.0 => 18.2.0
react-native: 0.71.11 => 0.71.11
react-native-windows: Not Found
npmGlobalPackages:
react-native: Not Found
Steps to reproduce
Create component/view with border radius and border bottom.
Snack, code example, screenshot, or link to a repository
<View
style={{
  flex: 1,
  height: 30,
  backgroundColor: 'red',
  borderBottomWidth: 7,
  borderColor: '#000000',
  borderBottomLeftRadius: 20,
  borderBottomRightRadius: 20,
}}></View>

the fix has been reverted https://github.com/facebook/react-native/commit/fd8a19d5e2bc00f29b3cd992d24790084cc34cbd

the fix has been reverted fd8a19d

Cc: @kelset

yeah it was reverted because, turns out, it was creating more problems than it solved 😅 if any of you can try to create a new PR, based on the previous fix, but that doesn't hit the regressions that we did experience here -> https://github.com/facebook/react-native/issues/37753 it would be great 👍


Cannot find command 'http'

C:\Users\Nitin> pip install --upgrade https://github.com/jkbrzt/httpie/archive/master.tar.gz
Collecting https://github.com/jkbrzt/httpie/archive/master.tar.gz
  Downloading https://github.com/jkbrzt/httpie/archive/master.tar.gz (262kB)
    100% |################################| 266kB 1.3MB/s
Requirement already up-to-date: requests>=2.3.0 in c:\programdata\chocolatey\lib\python3\tools\lib\site-packages (from httpie==1.0.0.dev0)
Requirement already up-to-date: Pygments>=1.5 in c:\programdata\chocolatey\lib\python3\tools\lib\site-packages (from httpie==1.0.0.dev0)
Requirement already up-to-date: colorama>=0.2.4 in c:\programdata\chocolatey\lib\python3\tools\lib\site-packages (from httpie==1.0.0.dev0)
Installing collected packages: httpie
  Found existing installation: httpie 1.0.0.dev0
    Uninstalling httpie-1.0.0.dev0:
      Successfully uninstalled httpie-1.0.0.dev0
  Running setup.py install for httpie ... done
Successfully installed httpie-1.0.0.dev0

Do I need to add something to path?
C:\Users\Nitin> http
http : The term 'http' is not recognized as the name of a cmdlet, function, script file, or operable program. Check the spelling of the name, or if a path was included,
verify that the path is correct and try again.
At line:1 char:1
+ http
+ ~~~~
    + CategoryInfo          : ObjectNotFound: (http:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CommandNotFoundException

Did you try http.exe?

Same error
Get Outlook for iOS
On Thu, Jun 23, 2016 at 9:57 PM +0530, "Ian Cordasco"<EMAIL_ADDRESS>wrote:
Did you try http.exe?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

I'm not a Windows user, but others might be able to help you better if you provide the following:

Version of Windows
Terminal/Command program
Version of pip and python
How you installed pip/python/etc.

I see chocolatey there and I don't know where that version of pip would install certain scripts.

Figured it out. Installed via chocolatey and found C:\ProgramData\chocolatey\lib\python3\tools\Scripts

can you explain how you did it?


Shuffling contents of Grid3D::Update_Hydro_Grid

This PR shifts some of the contents of from Grid3D::Update_Grid into Grid3D::Update_Hydro_Grid.
Overall, I think that this is a useful organizational change. I also need to do something like this on a local branch and it would be convenient to get this merged upstream as soon as we can since this modification seems like it could easily introduce lots of merge-conflicts.
I would recommend that the reviewer review the changes commit-by-commit:

The 1st commit literally moves everything after the Hydro-Integrator execution out of Grid3D::Update_Grid into the body of Grid3D::Update_Hydro_Grid (after the spot where it calls Grid3D::Update_Grid). I think this should be very non-controversial.

The 2nd commit renames Grid3D::Update_Grid to Grid3D::Execute_Hydro_Integrator, which is a more descriptive & self-explanatory name,  (I'm obviously happy to name it something else).

The 3rd commit moves Slow-Cell-Averaging & timestep-calculation to after the invocation of Grackle. They were already performed after other forms of cooling/chemistry; this is now more consistent and "correct". This is the only structural change to the control flow.

A side effect is that slow-cell-averaging & timestep-calculation are no longer part of the Hydro timing-region (this includes the Hydro_Integrator timing-region). This is addressed in the next commit

The 4th commit includes slow-cell-averaging & timestep-calculation back inside of the Hydro timing-region. The timing-regions are now consistent with the current version of dev.

I needed a somewhat hacky solution to do this - a more robust solution involves modifying the timing-functionality. I think this may be a little beyond the scope of this PR (I do have some ideas for refactoring this)
It also consolidates 4 ifdef regions down to 2.

To summarize: commit 1 is the most important change & commit 3 makes the only change to the control-flow.
If you take issue with any of the changes, I'm happy to update the PR to just include the first commit (or first 2 commits)

This all looks good to me. In practice, I wouldn't worry too much about the timing - we can just add another timer for the timestep calculation, which would be more intuitive, but I appreciate you keeping it consistent for now while we are still running scaling tests.


Can't carryover allowance with half days

When i try to carry over employees allowance with half day for example (1.5 day remaining from last year), the carryover makes it as full day (it becomes 2 remaining days). How can we fix that?

I had to change column type "carried_over_allowance" from int to float in the " user_allowance_adjustment " table, and it worked


Added analysing-static-footprint article to the blog

This PR adds a new article named analysing-static-footprint to the blog.

Transformed to PR https://github.com/espressif/developer-portal/pull/92
Please, close the issue


Update candle dependency to version 0.4

I'd like to use this library with the latest minor version of the core library! I can run this on a CUDA machine later today if manual testing is needed.

thanks! @tomsanbear


fixing README for Quorum Key Manager Quickstart

The target filename for docker compose was incorrect.

 Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Thanks @miseyu for the fixes, it's appreciated.

QKM and orchestrate have been deprecated from this repo and are out of scope. The projects however remain as is and tooling for them specifically will be evaluated and built accordingly


Songs API

Find a reliable song API to tap into for our app.

Mixes API added on current push to MovieAPI branch


⚠ ClamAV has degraded performance

In b4a62b1, ClamAV (https://spamassassin.apache.org/) experienced degraded performance:

HTTP code: 200
Response time: 67 ms

Resolved: ClamAV performance has improved in 45ba0c3.


Repackaging Azure Kubernetes

Required items, please complete
Change(s):

Repackaged Azure Kubernetes Solution

Reason for Change(s):

Update in Data Connector description

Version Updated:

Required only for Detections/Analytic Rule templates
See guidance below

Testing Completed:

See guidance below

Checked that the validations are passing and have addressed any issues that are present:

See guidance below

Guidance <- remove section before submitting

Before submitting this PR please ensure that you have read the following sections and filled out the changes, reason for change and testing complete sections:
Thank you for your contribution to the Microsoft Sentinel Github repo.

Details of the code changes in your submitted PR.  Providing descriptions for pull requests ensures there is context to changes being made and greatly enhances the code review process.  Providing associated Issues that this resolves also easily connects the reason.

Change(s):

Updated syntax for XYZ.yaml

Reason for Change(s):

New schema used for XYZ.yaml
Resolves ISSUE #1234

Version updated:

Yes
Detections/Analytic Rule templates are required to have the version updated

The code should have been tested in a Microsoft Sentinel environment that does not have any custom parsers, functions or tables, so that you validate no incorrect syntax and execution functions properly.  If your submission requires a custom parser or function, it must be submitted with the PR.

Testing Completed:

Yes/No/Need Help

Note: If updating a detection, you must update the version field.

Before the submission has been made, please look at running the KQL and Yaml Validation Checks locally.
https://github.com/Azure/Azure-Sentinel#run-kql-validation-locally

Checked that the validations are passing and have addressed any issues that are present:

Yes/No/Need Help

Note: Let us know if you have tried fixing the validation error and need help.

References:

Guidance for Detection checks
General contribution guidance
PR validation troubleshooting

Hello @v-vdixit  Data connector description link update did not reflected in the description
Other than that everything is ok

Checking on the fix as description is not getting updated.


Merge main into the momentum branch

Remember to keep it updated!
Unfortunately it can’t be automatically merged.

DON'T DO THIS!!! The main branch is for v2, and the momentum branch is for v3. We don't want to mix them up.


Ignore --> at end of HTML comments

When a TODO is added to a one line html comment you end up with --> at the end of the TODO text. It would be nice if this were automatically removed.
Not a huge deal, as it doesn't really affect readability, it would just be nice.
example
<header>
 <!-- TODO: Finish header -->
</header>

The TODO shows up as Finish header -->

Same goes for CSS' */.
I'm not great with regular expressions, but a positive lookahead should fix this. Something like (?=.*(\*\/|-->)$)? maybe.

My initial concern was the complexity of doing this across languages. I suppose we could start and do it for html, CSS and js.
I'd be happy to review a PR with this, though I'd want some unit tests with it.
I am not using atom at the moment (trying out vim) so I probably won't be building this feature in the near future.

@jamischarles You're gonna love this: https://en.wikipedia.org/wiki/Comparison_of_programming_languages_(syntax)#Comments

I want to keep the regexes in config as simple as possible, so before returning the match it internally strips known block comment endings for the major languages; C (*/), HTML, PowerShell, Haskell, Lua. This will hopefully cover over 90% of the languages used, where this is a "problem".


fix: respect pageSize and pageToken from gaxOptions

[x] Ensure the tests and linter pass
[x] Code coverage does not decrease (if any source code was changed)
[x] Appropriate docs were updated (if necessary)

Changes

Expose pageSize and pageToken in getTablesOptions
Respect pageSize and pageToken passed from gaxOptions
Values passed in options take precedence vs values from gaxOptions

It shouldn't break an existing code, I believe.

@AVaksman thank you 👌


Mixer support

Idea: Support Ergo mixer directly from within the app.

Closed in favor of implementing a general plugin system dapp developers can use to integrate their applications.


Ad hoc- KPGA weekly collection count table

@KELSEYDOWLING7 The PI at KPGA is requesting tabular format for number of collections per week reflected in Figure 6.15.

KPGA_Collection_Counts.csv
@erincschwartz

@erincschwartz  Did this need to be run weekly like Orland Park counts, or just the one time?


Drop most babel transforms

As discussed in #393

Why are the tests failing? It doesn't show any error

It looks like both Chrome and Firefox have some issue with assigning to module.exports.

What’s that line doing anyway? It looks like it runs a loop on itself

That's a dynamic require which is needed to load the tests files. Not sure why your change is conflicting with reassigning of  module.exports, but I noticed that we can remove this line anyway. Also in the main-packages.jswhich loads the test from /packages doesn't need this export assignment.

Can you apply that change to this PR?

It seems that the rebase has some effect on the build 🤷

Huh, that's interesting. I guess it had something to do with core-js: https://github.com/OctoLinker/browser-extension/commit/e56f75d5b5f22bb5d27ed46775d0d056d19600b2#diff-e00d3d45185f722f3df476b178fdd069


Add page: "usage guide (how to use the CLI for various scenarios)"

Under subsection "CLI Documentation"

Do you know what the scenarios are? :)

I don't know exactly what was meant by this but perhaps it's like "validating a packaged EPUB" or "validating a single file" or "validating an unzipped fileset" like what's described in the CLI docs

I rearranged the examples that were there and added one.


splatoon3schedule: support Tricolor Turf War for Final Fest

The data format has been changed for Final Fest. Update the app to handle this new format.


Add reporter options example

Description of the Change
I tried to add reporter options as reporterOptions or reporterOption and failed miserably for quite a while until I accidentally found out that it needs to be specified like in the example below.
Benefits
I think it's worth adding an example to the documentation to save other people's time.

We know you spent your time to figure this out, but this example configs aren't for explaining every option.
--reporter-option for XUNIT already described in our docs.

I know that, but what was surprising for me was the format. I was expecting to pass params like:
reporterOptions: {
     suiteName: "My Tests"',
     output: "./build/tests.xml"
}

like its displayed across all the documentation, while instead, mocha expected this:
'reporter-options': [
     'suiteName="My Tests"',
     'output=./build/tests.xml'
   ],

This was very confusing for me and I guess it would be good to have it in those examples. Especially your other examples are much larger: https://github.com/mochajs/mocha/blob/master/example/config/.mocharc.yml
I can change the example to be more generic like in .mocharc.yml with foo, bar..
What you think?

@juergba @tj @boneskull what do you think?

IMO it is rather confusing, see eg. #4142.
In our configuration files reporter-option has to be an array. When passing it directly to the Mocha  constructor, then it is an object.

My point exactly! There is no clear documentation about it, or at least I could not find it. However there is plenty documentation for passing an object to the constructor.
That’s why I strongly believe that adding example from this PR would help people...
Validation of mocharc structure would help too, but that’s a separate topic...

I understood what @pkuczynski pointed out. And I realized our .mocharc.yml is much verbose than others.
reporter-option is Array but each value is not same type with other Array type options. Therefore, IMO it is helpful for mocha newbies.

I can do that tonight if you want?

I just updated my PR to align with .mocharc.yml. The only thing I noticed is that .mocharc.js had package property which .mocharc.yml does not. Not sure if this is a bug?

I have not modified anything else than example/config/.mocharc.js. Possibly I misunderstood @outsideris comment and added all options from yaml to json. I personally feel that having all options would make it easier for newbies to understand. Please confirm if I should revert and stick to the simple version was here before?
I suggest updating other files in a separate PR once we align on .mocharc.js

Coverage increased (+0.07%) to 92.867% when pulling 13da3b3c5c3a3066a6a3525f9ca6634fd61ccd1d on pkuczynski:patch-1 into 54475eb4ca35a2c9044a1b8c59a60f09c73e6c01 on mochajs:master.

Build is finally green now :)

thanks!!

@juergba can you pls confirm your comments have been addressed; if so please merge

and by merge i mean "squash"


Ongoing warfare conversion

See how Vic3 saves store the carpet-occupation process and if it can be exploited to map EU4 occupations

part of #91.
But this is probably only part of the save file and thus presently not feasible. Neither the Brazilian nor the Texan wars start with occupied territory.

No, until such a time they fully rework military systems and warfare, it's impossible to map over war progress.


Resolve traits for infer using lang item infrastructure

As we already have the infrastructure for collecting lang items, use it to find the right traits when doing type-inference.

@kiljacken did you do sanity checking that this actually works with current stdlib?
Otherwise, r=me, that's a nice cleanup!

Looks good on my end:

bors r+


How to create Visual Studio Code Test package for automated testing of AL extensions?

Hello,
I have a created an AL extension APP for Business Central. Now i am in the process of submitting APP to Business Central app source. I have stuck in the place where i need to submit the App Test Automation.

Please guide me how to do that.
Thanks & Regards,
Sainath.

Hello,
You need to create a separate .app file that contains all your test codeunits. This test app needs to depend on your regular app (set in manifest/json). That test .app file then goes into that field. We then run these test .app files in our internal build systems for maintenance.
You can find information on creating this test app file here https://docs.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-extension-advanced-example-test
Ryan

@sainath654 thanks for filing this issue. Ryan's answer provides information how you can unblock yourself. I will now close this issue, please comment here if you still are stuck. If you experience another issue please open another GitHub issue.


The documentation for the latest version of this package is missing on Hackage.

The documentation for the latest version of this package is missing on Hackage. It's possible that Hackage wasn't able to build the package. see #103
Is it possible for you to manually upload the package using cabal upload --doc, please? Note that this requires cabal-install >= 1.24
Thanks!

Fixed!

Thanks!
On Sat, 29 Apr 2017, 3:25 pm Lee Pike<EMAIL_ADDRESS>wrote:

Fixed! http://hackage.haskell.org/package/ivory-<IP_ADDRESS>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/GaloisInc/ivory/issues/106#issuecomment-298172105,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0U3ADZaAEIXZOHPH4JtFvNqnTXA60Jks5r00hSgaJpZM4LpOgK
.


chore: dont panic on unsupported tag

temp fix until we have #2555 and can support these
but until then we shouldn't panic here

Codecov Report

Merging #2578 (81efcf4) into main (b40ce6f) will decrease coverage by 25.21%.
The diff coverage is 0.00%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
@@             Coverage Diff             @@
##             main    #2578       +/-   ##
===========================================
- Coverage   71.59%   46.38%   -25.21%
===========================================
  Files         490      490
  Lines       61674    61674
===========================================
- Hits        44157    28610    -15547
- Misses      17517    33064    +15547

Flag
Coverage Δ

integration-tests
18.16% <0.00%> (-0.02%)
:arrow_down:

unit-tests
39.54% <0.00%> (-26.80%)
:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

crates/interfaces/src/provider.rs
0.00% <ø> (ø)

crates/storage/provider/src/traits/state.rs
21.56% <0.00%> (ø)

... and 225 files with indirect coverage changes


Improvements to remote execution cell

Support secrets as node name in remote execution. However, it should default to plain text. Clicking on "A" swaps it to secret.

Let's add a help box at the top right, as we have in other smart cell, that clarifies the Erlang/OTP version of both nodes need to match.

I just ran through all of this -- tremendous feature ❤ I love getting that autocomplete in livebook, and I fully appreciate that it's important that this option be put right in front of us, so we don't do something that might be way worse.
To whoever adds that help box -- maybe that help box could also give them a snippet that shows them how to identify the relevant versions, in the case of the version mismatch error, because it's non-obvious that both of the following numbers absolutely need to match.
Or, maybe the smart cell could run+print out all of the below if it's possible to connect to the remote node, which would speed up diagnosis:
# on your system
:erlang.system_info(:version)
:erlang.system_info(:otp_release)

# on the remote system:
:erpc.call(node, :erlang, :system_info, :version)
:erpc.call(node, :erlang, :system_info, :otp_release)

Which, incidentally, is a bit of a sticky wicket if you install the Livebook Desktop app, because it may be unlikely to use the exact versions of everything that you're using in your remote app. It shows the elixir version in settings, for instance, but it might be nice if it showed the 2 relevant erlang versions too.
The one other thing I was thinking was -- is it technically possible/a good idea for Livebook to have the ability to download other erlang versions as needed? 😱 Ie, once we can successfully connect to the other node, detect and offer to download the appropriate runtime, and then launch the notebook in that.

Thanks for opening this issue, José – looks like you beat me to it.
Like @mrluc, I wondered if it might be possible for the Livebook to use a standalone Elixir runtime with a specific version to match the remote. But if I'm not mistaken, the Livebook communicates with its own managed runtime(s) via RPC too, so there's just no way around it. Everything has to match whatever version of Elixir Livebook was compiled with.

I found out what made it version specific to Elixir and I plan to fix that. So we should effectively only depend on the Erlang/OTP version. :)

That is excellent news!

We changed the RPC in #363 such that it no longer requires the same versions :)


Having to close floating terminal twice

Description
When opening a floating terminal I need to close it twice, with eg :q. The first time the actual floating terminal is closed but the slightly darker background is still there. Doing :q a second time brings the brightness back to normal. Anyone else having the same issue?
Expected Behavior
See above.
Actual Behavior
See above.
Neovim Built in Behavior
Not applicable.
Details

Reproduce
I don't have a minimal reproducable code example at this point but wanted to see if anyone else have the same issue.

Environment

nvim --version output: v0.5.0-dev+1425-g7da86f55a
Operating system: 5.12.11.arch1-1
lspsaga commit: cb0e35d

I seem to have same issue, the terminal is not closable without a keybinds(which I am having hardtime to get work) or press outside the terminal with mouse and run :q, which leaves a black vignette thing, which I again have to close

@mTvare6 Closing with :q works fine for me, it is just that I have to to it twice.

Using for example the following keymap works:
vim.api.nvim_set_keymap(
    't',
    '<A-d>',
    [[<C-\><C-n>:lua require("lspsaga.floaterm").close_float_terminal()<CR>]],
    {noremap = true, silent = true},
)


Add "unsrt" option to bibliography

Added the option "unsrt" to the document class to order the bibliography by appearance in text . Updated readme to match.
Also standardised thesis.cls to 4 space indentation since it was a mixture of tabs and spaces.

LGTM! 👍


refactor: use ar parameter instead of calculating aspect ratio

This PR serves as a follow up and (hopefully) resolves #73.
These changes refactor how the imgix-image component handles an image's aspect ratio, now using the imgix aspect ratio parameter ar= instead of appending a calculated height h= value based on the property. This should yield overall better performance, code cleanliness, and bring the component more in line with the imgix API.

Demo
Given the same component invocation, the URL constructed will be different but still render the same image.
{{imgix-image
	path="/users/1.png"
	width=100
	options=(hash ar="2:1"
}}

Example srcset URL

Before
http://domain.imgix.net/image.jpg?...&h=50&w=100

After
http://domain.imgix.net/image.jpg?...&ar=2%3A1&w=100

I hope that these changes will help clear up any confusion from #73.
Now that the imgix API supports the ar parameter, we can do away with all the unneeded aspect ratio calculations and the deprecated pathway. To simplify things further, we can treat aspect ratio as just another imgix query parameter rather than a property of the component. Although we are removing the deprecated pathway, I am grateful that @frederickfogerty used it as a way to nudge users towards using the W:H format, which will be necessary when using this new parameter. Hopefully this also clears up any concerns @Duder-onomy may have about mixing up formats now that we essentially only allow users to specify an aspect ratio in a single way.
NB: The imgix API specifies that ar will only work with fit=crop, which is a default parameter added in this library. Moving forward we will likely be removing that so as to be more aligned with the imgix API, which uses fit=clip by default. All of this points to a major release for ember-cli-imgix in the near term.
@frederickfogerty and @Duder-onomy, I'd love to get some input from both of you on this and see what you think.


TN-351 formatting test_organization.py through test_practitioner.py

Hello @John0Randolph! Thanks for updating the PR.

In the file portal/migrations/versions/8efa45d83a3b_.py, following are the PEP8 issues :

Line 53:80: E501 line too long (83 > 79 characters)
Line 68:80: E501 line too long (81 > 79 characters)
Line 76:80: E501 line too long (81 > 79 characters)
Line 79:80: E501 line too long (88 > 79 characters)

In the file portal/views/portal.py, following are the PEP8 issues :

Line 1099:80: E501 line too long (86 > 79 characters)
Line 1117:80: E501 line too long (83 > 79 characters)
Line 1135:80: E501 line too long (83 > 79 characters)


Complete User Profile Creation

### CreateProfileFragment :

Apt Image/ alternative graphics to be used to replace the current image (BNPL) in the layout.
Fix deprecated function(s) to select image from device storage.
If agreed, this class can also add user's social media accounts to the app.

Hiya man, I got a notification about this. I will get back to ya later. by the way I think our app needs a redesign, I would love a new meeting session to rethink the app.


enomem when server cert has no CN

When trying to mitm a server whose certificate subject only has: "O=my org", sslsplit stop logging after request because ssl_x509_names_to_str return null on pxy_srcssl_create and thus set ctx->enomem to 1.

Thank your for the bug report!


Get informations without download the ics file

Hello and thanks for trying to solve the issue of teams and outlook appointments.. really a pain in the a**
Is it possible (i really don't know how it works) to have the appointments informations without download the .ics file (and then import it in a calendar application) ?
Thank you !

I am not quite sure what do you want. You can import the .ics file in the most calendar applications.

I will clarify then :)
In fact, my colleagues use an online diary or paper diaries. When we send them an Outlook invitation, they just want to see the date, and the name of the event and potentially the description, and then transfer it to their diary by hand. So they don't go through a desktop calendar application.

I'm glad someone tries to solve the issue with teams invitations.
It would be nice if instead of having to write the file to a folder and then import it, we can just directly open the ICS file and import it to the desired calendar.
I give you another idea. Usually Teams writes in location "Meeiting in teams", and it should write the link to the meeting ;)

@quentinDupont Thunderbird is already showing these informations. I am using the version 91.8.0. I think this was missing in older Versions:

@jlanza The problem for me was that my company isn't using the microsoft email servers. With that no invite was imported in the outlook/teams calendar. When I am receiving an invite, I am importing the ics file in the outlook calendar. The outlook calendar is the same as the teams calendar. https://outlook.office.com/calendar/
For my use case is an automatic opening of the ics file not practicable and I am not quite sure that it is not possible to open files with an thunderbird extension.

@Leworoe if you find the way not to have to store the file and then import it it would be great. I mean to directly add to the calendars. Yesterday I tried but I was no able. I always have to write the file to disk and them include the invitation :(

@quentinDupont Thunderbird is already showing these informations. I am using the version 91.8.0. I think this was missing in older Versions:

Hello,
I'm confused. Do you use other add-on to get this ?
I use Thunderbird 91.8.1 version.
For Gmail invite, i do see this screen. But not for Outlook invit

@Leworoe I love your attachment and I have a similar request. Is it possible to add a button to download and open the file?
My use case is that I want to import those events to the Calendar app on macOS instead of Thunderbirds integrated calendar. So I would love to just click the button and macOS Calendar would just ask me where to import it.
Currently I need to download the file to the Downloads folder and then manually double click it.


unimplemented without Void placeholder produces deprecation warning

The following declaration produces a deprecation warning on the Swift 6 language mode only. I'm aware that the placeholder-less methods were deprecated but this seems to have only appeared after #114 was released.
var onCancel: () -> Void = unimplemented("ViewModel.onCancel") // warning: 'unimplemented(_:file:fileID:function:line:)' is deprecated: renamed to 'unimplemented(_:placeholder:)'

I can of course resolve the warning with the following but I don't think the intention is that this is necessary.
var onCancel: () -> Void = unimplemented("ViewModel.onCancel", placeholder: ())

This should be fixed by #123! We'll do a release soon.


Default color of hv.Bounds changed

Probably other elements also changed color, but hv.Bounds went from c to b.
EDIT: just checked, Path changed too.

This was fixed by the same fix as in #162 and #164 .
"@jlstevens helped me on Gitter. It was because I used the rc file like in this example: https://github.com/ioam/holoviews/issues/92#issuecomment-104900298"

Same issue as #162 and should now be resolved.


Added implicit conversion from Unit to Html

The conversion is useful in cases when we don't want to construct any Html but want to leave it empty. It is is expected by users of scalatags or similar libraries.

Codecov Report

Merging #1204 (4da0615) into main (0d840bb) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1204      +/-   ##
==========================================
+ Coverage   58.57%   58.63%   +0.05%
==========================================
  Files          74       74
  Lines        2460     2461       +1
  Branches       79       66      -13
==========================================
+ Hits         1441     1443       +2
+ Misses       1019     1018       -1

Impacted Files
Coverage Δ

zio-http/src/main/scala/zhttp/html/Html.scala
71.42% <100.00%> (+21.42%)
:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d840bb...4da0615. Read the comment docs.

thanks @nartamonov !


Add i18next.

Add i18next to be able to have captions in several languages. For now, english (en-US) and norwegian (nb-NO), and only for intro texts (intro.js). There are several things that are not translated yet, including the html in the jade layouts. See comments in i18n.js.
Hopefully, if all the text in codeclub_lesson_builder can be included using i18n, perhaps the nb-NO-branch can be merged with master and then deleted.

Note that the default language is now set to norwegian (see option "lng" in i18n.js). Since this is the master branch, it should perhaps be changed to english?

Nice work!

Hopefully, if all the text in codeclub_lesson_builder can be included using i18n, perhaps the nb-NO-branch can be merged with master and then deleted.

Then we need to put the configuration in the lesson repo? #155 may make this easier.

Since this is the master branch, it should perhaps be changed to english?

Yes.
Would you like this to be merged now, before moving the layouts over to i18next?

Why do you say that the config would need to be in the lesson repo? Were you thinking that the lessons also should be in both languages?
And you are probably right, I should do the layouts before merging in the changes. I was thinking of using https://github.com/i18next/jquery-i18next for this.

config ...

I'm thinking that you need to specify the language somewhere. But that might be better to have in the front end? Might be what you have thinking all along?
Instead of jquery, what about defining i18next in the layout, then build both languages?
https://github.com/arve0/codeclub_lesson_builder/blob/master/build.js#L50

The code that I now added to this pull request will build the static files once (currently english, but by changing "lng" in build-with-i18n.js it can build a different language). The language of the rest can be changed in the client. It still means that a manual change + rebuild needs to be done if one wants to change the language of the website, but it is a step on the way.
I want to be able to change the language on the client, on the fly. I have tried to include the plugin metalsmith-i18n, but it seems like it is using an older version of i18n and did not work to my satisfaction. (I might of course have misunderstood how metalsmith-i18n works and done it suboptimally, but...)
I think, at this point, the best option is NOT to build all the html-files (via metalsmith and jade) twice, but rather find a way to only have one copy of each (since it is hard to make sure all the links are correct for each language, and also it seems unnecessary to have two of every html-file).
I have two ideas so far. One is to first create tags with IDs for all the text that needs translation, and then make metalsmith "hardcode" all the translations into a script in the head of layout.jade, and have the same script change the tags with the given IDs. Then one can change the language with e.g. "?lng=nb-NO" (and perhaps set it in a session variable, or even as a cookie) --- and perhaps add a couple of buttons/links/flags to change the language as well.
The other idea is to change the front to use React. That way React can rerender the page with the new language whenever it is changed by the client.

Hm, I see that the build failed. Looking into that...

I have now added the captions in the html-files (created by metalsmith and jade) by using the attribute "data-i18n" in the tags that need captions (see i18n.js for details on how to include translations for both the innerHTML and attributes). This way we don't need to compile two sets of html-files, and one can easily switch on the fly. The language is stored in a cookie so that it remembers it the next time you go to the webpage.
I have also added flags in the topmenu to quickly switch between english and norwegian. These can easily be turned on and off in config.js (showFlags = true og false), and the rebuild.
I have translated most of the captions, but I think there are a couple that remain. These should be easy to add later.

I think this pull-request is ready to be merged. Hopefully we can get rid of the nb-NO branch now!

Thanks, I see you have put in a lot of work! As this change is quite comprehensive, I'll need some time to review it.
A few initial thoughts:

We should document this change.
Is initial "build" possible in the templates?
Related to point above, is there any SEO we should think about? In example title is a bit unfortunate, as "kodeklubben oppgaver" would be a top search term. kodeklubben is in the url, so this might not change search results. But it will change the search result title, which is a part of the authenticity.

Completely agree! Do you have any thoughts on where? In the readme.md?
Not sure what you mean with this question. Do you mean that there should be default (hardcoded) texts in a language given by the build config? (Right now, the texts are empty until the page is loaded, and then filled in). If this is the question, then it is not possible right now, since I removed the use of i18n in the build-script, but if I reintroduce it from an earlier commit, then it should be doable.
I don't really know much about tuning SEO, and not sure what you mean when you say that "title" is unfortunate. The document.title should be the same as before, it is only if it is missing from the metalsmith templates (in oppgaver/src) that the title in the translation.json is used.

I've started reviewing. A bit hard to follow the flow, but getting there. Hope to get it done tonight, if not I will finish it in the week to come.

I'm thinking of writing some few words in a separate file, like STRUCTURE.md. I'll take a stab at it when finished reviewing this.
What I'm thinking of is adding i18n_t('key') to layouts. Then in client side, only do i18n_t('key') if language is changed explicit (clicking the flag). Example:

// from
a.download(href=link.replace('.html', '.pdf') data-i18n="title=layout.downloadPdf")
// to
a.download(href=link.replace('.html', '.pdf') data-i18n="title=layout.downloadPdf" title=i18n_t("layout.downloadPdf"))

Search engines do not run JS. Old:

vs new:

PS: Thanks for the the work this far.

A few comments:

This can relatively easily be done by reintroducing the code I had in commit d5a3623fd2cdbb66c8dc6014ed911b46a91c243c.

Ah, my bad, I thought that "pipe" was a logical "or", which is why I removed it. Easily reinserted (especially if the code mentioned above is reintroduced).

Do you want med to do this? It shouldn't take too much time.
Also, feel free to bug me on Facebook if you have any questions! :)

Do you want med to do this? It shouldn't take too much time.

Sure, though I think it will be a good practise for me :wink:

Personally I must admit that I am more used to 4-space indent, but it seems like 2-space has been used a lot already, so seems fair to keep it that way. Btw, reason I didn't fix the indents in the last PR was that I wanted to make as few changes as possible. But good to have it properly indented.
One small suggestion: What do you think about adding the option " load:'currentOnly' " to i18n? If not, then a lot of warnings pop up in the console (due to the whitelist) when using debug:true. I am going to merge the PR anyway, we can always add this afterwards.

Funfact, in Github's top repositories, 60% are 2 spaces, 27% are 4 spaces. But more seriously, this should be fixed by a tool. Manually indenting is sooo last year :wink:

I also noticed that layout.jade has no hardcoded translations in it yet. Also, the title is still "Oppgaver" and not "Oppgaver | Kodeklubben". Do you want me to fix any of these after the merge before you do the merge of the original PR?

I will add this.

Ref "force a language": I suggest to set "showFlags" to false (in config.js), thus hiding the flags. It will still be possible to change language using ?lng=en-US, but it won't be available by clicking. But this way, we have both translations there all the time, and don't need two separate branches for the languages.
In order to make sure it always uses norwegian, we could add lng:'nb-NO' to the i18n.init() options, thus choosing this automatically (unless it was set to english previously and stored in localstorage).

60% is 2-spaces? Interesting! Also, what are the remaining 13%? 3 spaces? Tabs?

60% is 2-spaces? Interesting! Also, what are the remaining 13%? 3 spaces? Tabs?
Show all checks

In javascript, yes. Remaining are tabs. Another funfact, 4 spaces is 92% in python, but Ruby is 100% 2 spaces.

In order to make sure it always uses norwegian, we could add lng:'nb-NO' to the i18n.init() options, thus choosing this automatically (unless it was set to english previously and stored in localstorage).

I think specifying with lng would be sufficient.

Thanks @NorwegianKiwi! nb-NO branch can be deleted after the layouts/styles have been merged back into master.


chore: disable all modules on lack of initial api version

Instead of failing to initialize the Client, simply pretend it was impossible to initialize any module. This should be easier to gracefully handle downstream if the Federation is e.g. temporarily down.
Re #4887

Hmm, I wouldn't have expected this PR to solve the root issue but this fixes #4999. Others can verify by running just test-upgrades "v0.3.0 current" vs this branch and master.

I don't know the details about the problem there, but that's surprising. :D
I guess the upgrade tests's problem must have been inability to negotiate the api versions to use on first start, and the test there doesn't really check if it can do anything with the Client, just needs it to initialize.

Dang, looks like it was too good to be true. I reran the upgrade tests in a loop vs this branch and it failed 60% of the time.
Doesn't impact this PR though, the diff still looks good 👍


04/22/2018 18:08 : Create Pull Request for branch prTesting_2018042218 -> master

:white_check_mark: Validation status: passed

File
Status
Preview URL
Details

split/updateTestCon.md
:white_check_mark:Succeeded

For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.


Using #with_content on pass through slots throws 'undefined method with_content for nil:NilClass'

In lieu of a pr I have a suspected cause, detailed at the end of this issue.
Steps to reproduce
Using with_content on a pass through slot with no arguments, like what is documented here.
Expected behavior
The content argument passed into with_content is rendered as the slot.
Actual behavior
The error undefined method with_content for nil:NilClass is raised.
System configuration
Rails version:
<IP_ADDRESS>
Ruby version:
3.0.0
Gem version:
2.31.1
Suspected cause
SlotableV2's defined method for a slot, seen here, checks to see if the slots caller passed in args or a block and uses that to determine the response.
If you call the slot without args or a block it's value is the result of get_slot(slot_name) which is the value nil returned on line 202 of that file.
If I pass args into the slot <% modal.header(classes: nil).with_content(title) %> it renders the slot correctly.
Potential solution
I'm happy to open a PR to fix this, but I'm not sure what the solution should be. To get access to with_content the slot's method will need to return an instance of SlotV2 whether or not arguments/a block are passed in. I don't know if that means the parts of set_slot responsible for initializing the slot need to be extracted to a method both get_slotandset_slotcall or ifget_slot` should just be removed completely. I could also be way off base with my proposed solution.

I realized that this wasn't occurring on collection slots when passing in content to the singular form of that slot (ie a menu_item of the slot menu_items). I went and checked and found that the singular version of a collection slot always returns the result of set_slot`, see here.

I ran into this exact problem working on https://github.com/jonspalmer/view_component_storybook/pull/68
I like the proposed solution a great deal.

@BlakeWilliams Thinking about this a bit more I'm not convinced that your solution will work. I'm worried that it forces all slots to have to be set before they are read. Otherwise any optional slot will get instantiated incorrectly when it's read at render time.
A few other thoughts on solutions:

Add a magic context argument @joelhawksley and I touched on this here: https://github.com/github/view_component/pull/1008#issuecomment-882874968
Could we add a "bang" version of the slot method that was always the set_slot behavior and never a getter? It's a little quirky but not totally inconsistent with bang methods mutating state

This has been fixed with the new Slots API that split out the getter and setter ❤