Type generation for "modularly designed" libraries is hard

I'm reaching out to you because over at  ReactiveX/RxJS we've been using TypeScript, primarily as a means of generating "correct" .d.ts output for our consumers. However, because of the design of our library, that dream isn't really being realized, because of issues outlined here.
The specific problem is that we have a need to export a class with type information about what methods it could have. We end up jumping through some strange hoops to make this happen. The reason the architecture of RxJS 5 works this way is simple: not every Rx project needs every Rx operator, and most project build systems don't have granular tree-shaking to remove all of the RxJS operators that a project isn't using. The "low tech" solution is to just provide a plain Observable object to people and let them import operators and decorate the prototype on the one they use.
I think TypeScript has had some real benefits for us, so I don't want to walk away from it if it still makes sense. But if the primary reason we were using it was to generate .d.ts files, and we end up jumping through hoops to generate those .d.ts files, which cause us to have to workaround TSC complaints, we (at RxJS) should at least re-examine our approach.
I'd really appreciate experts from your community reviewing the issues we've had and offering up suggestions. Ideally, the RxJS project's woes can offer up to you an opportunity to make the TypeScript experience more intuitive in situations like this.
Thanks for your time.

We'd love to help solve your problem. First off, let me ask what you would do differently in just plain ES6? In curious if there's something the type system is preventing you from doing, or whether the friction stems from the way ES6 modules work? For example, some would argue that it is an anti-pattern for module imports to have side effects on objects defined in other modules, such as the Observable prototype, but perhaps that's the only meaningful way to implement the kind of modularity you need. TypeScript currently supports type merging across namespace (formerly "internal module") boundaries, but we haven't gone there with ES6 modules because of the side effect smell. Is it something we should consider?

BTW, I should add that the pattern of having a library add members to other types (or have the illusion of doing so) is precisely why we introduced extension methods in C#. But in C# it is purely syntactic sugar over rewriting instance method calls into static methods calls, passing the receiver object as the first argument (i.e. obj.foo(...) gets rewritten to Library.foo(obj, ...)). Unfortunately this likely isn't an option for TypeScript as it doesn't really have a counterpart in JavaScript.

First off, let me ask what you would do differently in just plain ES6?

Honestly, not much. There would be less "adding method stubs" (per the issue I linked), and less "casting", I suppose. It might just be easier to maintain that code and a manually written .d.ts file. I was initially worried about keeping those two things in sync, however, outputting types our users want from our build has proved challenging.

curious if there's something the type system is preventing you from doing

There's two things I'm running into. I can't output type information for methods that could optionally exist on the class. ... however, I can add property "stubs" for those methods. ... BUT... If I do that, then any subclass of that class has to implement those as instance properties... but I don't want instance properties because I want those function references on the prototype, not the instance.
class Foo {
   someMethod: () => void
}

// this doesn't work
class Sad extends Foo {
  someMethod(): void {
  }
}

// this works, but sucks, because I'm making a new function copy per instance
class Better extends Foo {
  someMethod = function() {
  }
}

// this works in a more desirable way, but sucks because it's ugly
class Betterer extends Foo {
}
Betterer.prototype.someMethod = function() {
}

Ideally, the first subclass Sad would "just work", because there doesn't seem to be a reason in a prototypal structure, that a type system should differentiate between an undefined instance property and a property on the prototype with a function in it (which is what a class method is). I can see why it would complain if the superclass defined an instance method and the subclass defined a method on the prototype, but if it's undefined in the superclass it doesn't seem like it should be an issue. I'm clearly just putting it there for the type information.

Unfortunately this likely isn't an option for TypeScript as it doesn't really have a counterpart in JavaScript.

The closest thing I can think of is ES7 function bind. But you'd have to add some additional syntax in TypeScript to let the compiler "know" what a standalone function could "bound" too. Like:
extension function add<T as string|number>(x: T) : T {
  return this + x;
}

So if I have a string foo and I type foo., VSCode could know to show me add(x: string): string as an option.
I'm spitballing. Designing languages is hard, I wouldn't pretend to know the edge cases around something like that.
It's probably a lot to commit to for a  Stage 0 proposal feature... but if ES7 function bind doesn't make it into JavaScript I'll eat my hat.

Thanks again to you and @RyanCavanaugh for your time. I really appreciate it.

I can't output type information for methods that could optionally exist on the class

Does this do what you'd like?
interface MaybeMethods {
	someMethod?(): void;
}

class Foo implements MaybeMethods {
	// some core logic, perhaps
}

// All is good
class Sad extends Foo {
  someMethod(): void {
  }
}

@RyanCavanaugh I don't think the Foo.d.ts output would reflect the possibility of a someMethod on Foo though... I think it will say "well, it could be there, but it wasn't so no need to put it in the .d.ts". I guess I could add stubbed methods on Foo maybe?
class Foo implements MaybeMethods {
   someMethod() { throw new Error('not implemented'); }
}

It's just a lot of extra stuff for what we need to do. Which is why I'm leaning toward manually creating .d.ts files and going to plain ES6. That comes with it's own drawbacks though. :\ It's a bit of a pickle.

In TypeScript 1.6 we implemented the ability to merge class and interface declarations (see #3332) so extension libraries can add methods to base libraries. We restricted this capability to ambient classes only, but perhaps we should relax that restriction (there wasn't a strong reason for the restriction in the first place). This would allow you to do the following:
// Optional methods
interface Foo {
   someMethod(): void;
   anotherMethod(): void;
   yetAnotherMethod(): void;
}

// Actual implementation
class Foo {
    regularMethod(): void {
    }
}

class Better extends Foo {
    someMethod(): void {
    }
}

It actually works right now, except you get errors on the two declarations of Foo stating that only ambient classes and interfaces can merge. But other than that, everything works the way you'd expect and Foo has the merged set of methods.

That sounds like a decent solution, actually.

@mhegazy Opinions?

Here is the change to stop reporting the errors: #5290; I think we also need a way to allow modules to modify global types (something along the lines of #4166).

:+1:

This change should be in typescript@next (1.7.0-dev.20151017) later tonight

:+1:

Longer term we'd also like to fix the deeper problem of not being able to extend types in other modules. There's some information on our thinking in the design notes, but basically we'd like to get to something like this:
// module myoperator.ts
import { Observable } from "../observable";

declare module "../observable" {
    // Extend the Observable<T> interface
    interface Observable<T> {
        myOperator(...): Observable<T>;
    }
}

function myOperator(...) {
    // Implementation of operator
}

Observable.prototype.myOperator = myOperator;

So, when the "myoperator" module is imported it automatically extends the Observable<T> type and the Observable prototype.
Hope this makes sense.

Should be 1.8.0-dev.20151017.

@DanielRosenwasser ... What is in that tagged version?

just to clarify, we have forked the release 1.7 into its own branch, and now the version of master, and thus of the nightly builds moved to 1.8. the version of the first nightly (typescript@next) with #5290 is 1.8.0-dev.20151017. TS 1.7 will have the change as well.

As mentioned in https://github.com/Microsoft/TypeScript/issues/5292, modules should be able to add declarations in other modules as well as the global scope. We will need a different issue to track the design and implementation of that, but keeping this open for now.


Allow multiplie targets

Webpack can't use multiplie targets in one config https://webpack.js.org/concepts/targets/#multiple-targets, i think this patch can resolve this problem.
Example configuration

module.exports = {
    webpack(serverConfig, options) {
        const clientConfig = {
            target: 'web', // <=== can be omitted as default is 'web'
            entry: {
                client: ['src/client/index.js']
            },
            output: {
                path: 'build',
                filename: '[name].js',
                sourceMapFilename: '[name].map',
                publicPath: '/',
                libraryTarget: 'commonjs'
            }
        };

        return [serverConfig, clientConfig];
    }
};

this is a pretty cool idea

Before finding this PR, I tried to do exactly this. My use case is I need a slightly special build for a cli "bin" and would have loved this to be supported
Any chance of seeing this one day ?


file name offset

This issue is less urgent than the other one I just wrote.
In my personal ScribusGenerator fork I have had an "offset" for years with its own input-box in the GUI, which allows me to run several batches of "file generations" from different CSV data files and have them all saved in one huge folder, for easier printing by some other scripts of mine.
So instead of generating each time the same files-names like 1, 2, 3 or better 001, 002, 003 (see my previous issue) I used to have the option to enter an offset of 500 (for example) and then generate for example  501, 502, 503.
My offset is not string-added, it is mathematically added to the index; so it works fine even if I generate many files:  500 + 1 = 501;  500 + 233 = 733 etc.  So when I create another batch, I just have to look in my file-browser and use an appropriate-size offset. And this allows me to have all my output together and still see which file is part of which batch. Very useful to me.
Again my question: Is this feature interesting to other users?
And if yes, should I attempt to work on the code (might take me several days and I do not want to create conflicts with other work) or would the core team consider doing it properly?
Of course I can show the few lines of how I did it back in a very old version of ScribusGenerator, it is not too complex. But things have matured so much here, that I fear my skills are no match for the current version.
Please let me know what you think,
Martin

Do you need the numbers of all generated files to be a strict sequence of numbers with no gaps?
If not I would propose to use the features that already exists to use some variables in the output file name, for instance, adding a variable "prefix" or "series" to your data and using the following string as output file name:  "myfile_%VAR_series%_"
I personal like to use uppercase letters in my series (AA < AZ < ZZ), but you can use numbers all the same.
I'd rather avoid adding new options unless they have big added value, but am happy to do it if it's really needed.
hope this helps,
B.

after reading your post more carefuly I gather you may not want to have that "offset" in the CSV data. Then just put it manually in the output file name, increasing the last series number you used by one.
eg, if last file was  myfile_BJ_ 078.sla, then the output file name used for next generation might be myfile_BK_

Dear berteh,
thank you for kindly looking at my request #190.
You are right, that I do not need a very specific list of file names in our batches. It would just be great for my existing scripts, if things would not change dramatically. Otherwise I need to adapt my script for pagination and for batch-printing.
Your ideas gave me more of my own ideas:
I just tested to put nothing but a prefix in the "file name box" like "22_" and hoped that since I did not use any regular %VAR_something% at all, that maybe the new Scribus Generator would kindly add the good ole index numbers. But instead it created all 100 output files at that same file name, writing one over the other. Was just a test. Did not work as hoped.
I have total control over our data files, since I generate them myself. So I have to consider whether I prefer hacking the Phython code again to have my offset feature, or whether I just quickly hack each CSV file with a numbering to my liking and use that for file name purposes. I see several options for that.
I will let you know here. For now, since you clearly wrote "I'd rather avoid adding new options unless they have big added value" I will withdraw my request #190. Like I had written, this one is less urgent.

the overwriting of output files instead of increment is a bug that will be fixed very soon, sorry.

variable names should now be substituted as expected, along with a dedicated variable that gives the counter (%VAR_COUNT%) in output file name.
kindly test and feedback if this works for you in python3 branch (scribus 1.5.6+)
doc: https://github.com/berteh/ScribusGenerator/blob/python3/README.md#dynamic-output-file-location
(improvement to the documentation are welcome too, if you have a way to make it more clear in short, thanks.)


Error when loading schema from graphql.config.yaml - schema.getQueryType is not a function

Describe the bug
Attempting to run graphql linting relying on graphql.config.js currently appears to fail with the error:
$ DEBUG=graphql-eslint:* eslint empty.graphql
  graphql-eslint:parser cwd '.../repo' +0ms
  graphql-eslint:graphql-config options.skipGraphQLConfig: undefined +0ms
  graphql-eslint:graphql-config Graphql-config path '.../repo/graphql.config.yaml' +0ms
  graphql-eslint:schema Loading schema from 'https://countries.trevorblades.com/' +0ms
TypeError: schema.getQueryType is not a function
    at getRootTypeMap (.../repo/node_modules/@graphql-tools/wrap/node_modules/@graphql-tools/utils/index.js:713:30)
    at Object.memoized [as getRootTypeMap] (.../repo/node_modules/@graphql-tools/wrap/node_modules/@graphql-tools/utils/index.js:519:30)
    at generateProxyingResolvers (.../repo/node_modules/@graphql-tools/wrap/index.js:15:31)
    at Object.wrapSchema (.../repo/node_modules/@graphql-tools/wrap/index.js:80:31)
    at UrlLoader.loadSync (.../repo/node_modules/@graphql-tools/url-loader/index.js:801:30)
    at loadFileSync (.../repo/node_modules/@graphql-tools/load/index.js:115:46)
    at .../repo/node_modules/@graphql-tools/load/index.js:399:25
    at Object.runAll (.../repo/node_modules/@graphql-tools/load/index.js:245:17)
    at collectSourcesSync (.../repo/node_modules/@graphql-tools/load/index.js:291:11)
    at loadTypedefsSync (.../repo/node_modules/@graphql-tools/load/index.js:540:21)
error [graphql-eslint] Error while loading schema
 TypeError: schema.getQueryType is not a function

Oops! Something went wrong! :(

ESLint: 8.11.0

Error: Error while loading rule '@graphql-eslint/require-id-when-available': Rule 'require-id-when-available' requires 'parserOptions.schema' to be set and schema to be loaded. See http://bit.ly/graphql-eslint-schema for more info
Occurred while linting .../repo/empty.graphql
    at requireGraphQLSchemaFromContext (.../repo/node_modules/@graphql-eslint/eslint-plugin/index.js:38:15)
    at Object.create (.../repo/node_modules/@graphql-eslint/eslint-plugin/index.js:3066:24)
    at createRuleListeners (.../repo/node_modules/eslint/lib/linter/linter.js:922:21)
    at .../repo/node_modules/eslint/lib/linter/linter.js:1104:31
    at Array.forEach (<anonymous>)
    at runRules (.../repo/node_modules/eslint/lib/linter/linter.js:1041:34)
    at Linter._verifyWithoutProcessors (.../repo/node_modules/eslint/lib/linter/linter.js:1389:31)
    at Linter._verifyWithConfigArray (.../repo/node_modules/eslint/lib/linter/linter.js:1729:21)
    at Linter.verify (.../repo/node_modules/eslint/lib/linter/linter.js:1471:65)
    at Linter.verifyAndFix (.../repo/node_modules/eslint/lib/linter/linter.js:1953:29)

To Reproduce
Steps to reproduce the behavior:
mkdir repro
cd repro
npm i<EMAIL_ADDRESS><EMAIL_ADDRESS><EMAIL_ADDRESS>cat << EOF > .eslintrc.js
module.exports = {
  root: true,
  overrides: [
    {
      files: ['*.graphql'],
      plugins: ['@graphql-eslint'],
      parser: '@graphql-eslint/eslint-plugin',
      parserOptions: { operations: './*.graphql' },
      extends: 'plugin:@graphql-eslint/operations-recommended',
      rules: { '@graphql-eslint/require-id-when-available': 2 }
    }
  ],
}
EOF
echo 'schema: "https://countries.trevorblades.com/"' > graphql.config.yaml
echo 'query DeleteMe { me { __typename } }' > empty.graphql
DEBUG=graphql-eslint:* eslint empty.graphql

Expected behavior
Linting errors related to the actual query
Environment:

Mac OS Monterey (M1)
<EMAIL_ADDRESS><EMAIL_ADDRESS><EMAIL_ADDRESS>NodeJS v17.5.0

Hi @mogelbrod, thank you for your report and reproduction!
I was able to reproduce, seems like the error goes from graphql-tools and not from graphql-eslint.
I leave this issue open until we fix graphql-tools.
Will back soon 🦸♂

@B2o5T Got it! Do you want me to file an issue on https://github.com/ardatan/graphql-tools/issues?

@mogelbrod no need, I already told 👌 fix with test https://github.com/ardatan/graphql-tools/pull/4334 was merged, so I’ll release a fix for graphql-eslint also soon.

fixed in<EMAIL_ADDRESS>


bugfix(Callout): switch button type

Button type switched to tertiary

@ghlost This one looks like it needs the Storyshot regenerated.

@johnpauldavis updated the snapshots!


[BEAM-11312] Log cloud build url and enable kaniko cache in sdk_conta…

…iner_builder
This PR adds logging for the build history url in the case the google cloud build run failed, and also enables kaniko cache for better build performance as https://cloud.google.com/cloud-build/docs/kaniko-cache suggested. Experiment shows with kaniko cache, the build time for is greatly reduced after initial build. This helps user to launch job faster with prebuilt python sdk workflow.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

[ ] Choose reviewer(s) and mention them in a comment (R: @username).
[ ] Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
[ ] Update CHANGES.md with noteworthy changes.
[ ] If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

R: @tvalentyn

friendly ping for review

Could you please add some details to the PR description about Kaniko and why are we want to use it? Thanks.

friendly ping for review
Thanks, I think I missed this PR.

Could you please add some details to the PR description about Kaniko and why are we want to use it? Thanks.

Done.

Could you please add some details to the PR description about Kaniko and why are we want to use it? Thanks.

Done.

PTAL

Thanks. It seems that caching may improve the startup time, and be useful for users who frequently launch the same pipeline. However I think caching may result in a difference in behavior. Questions:

Is it possible that caching will result in a stale image that users will perceive as undesirable and the behavior will be difficult to debug to users or support folks? For example, if a user pipeline depends on a latest version of a dependency X in pypi. Perhaps a dependency they control. They have a pipeline with a setup.py that has an open install_requires bound dep>=1.0.0 < 2. They run the pipeline, then push dependency to pypi and run the pipeline again, expecting a change in behavior. Kaniko will not rebuild the image in this case, right? What are your thoughts on that?

During runtime with prebuilding workflow enabled, how visible is it to the user that the cached layers are reused and not rebuilt?

I think we should document the prebuilding feature in Beam docs, and reflect the caching behavior and associated TTLs. What is a plan for that?

Would customizing the TTL or adding a no-cache option make sense? We are using default 2 weeks TTL, right, see: https://cloud.google.com/cloud-build/docs/kaniko-cache#configuring_the_cache_expiration_time.

cc: @emilymye who may also have feedback.

My main worry with this is similar to Valentyn's, that Kaniko detects changes based on the Dockerfile and the majority of the time our changes to the image are not to the actual Dockerfile itself.

Thanks. It seems that caching may improve the startup time, and be useful for users who frequently launch the same pipeline. However I think caching may result in a difference in behavior. Questions:

Is it possible that caching will result in a stale image that users will perceive as undesirable and the behavior will be difficult to debug to users or support folks? For example, if a user pipeline depends on a latest version of a dependency X in pypi. Perhaps a dependency they control. They have a pipeline with a setup.py that has an open install_requires bound dep>=1.0.0 < 2. They run the pipeline, then push dependency to pypi and run the pipeline again, expecting a change in behavior. Kaniko will not rebuild the image in this case, right? What are your thoughts on that?

I think kaniko cache works the same way as docker layer cache, that is to say, if the locally downloaded artifacts changed(or requirements.txt, setup.py changed) it will actually change the COPY step in the prebuilding workflow. There will be no valid cache layer since the artifacts copy step and a new image will be rebuilt. (also verified through my own experiment with changing requirements.txt)

During runtime with prebuilding workflow enabled, how visible is it to the user that the cached layers are reused and not rebuilt?
There will be log entries "No cached layer found for cmd ..." in the cloud build log.

I think we should document the prebuilding feature in Beam docs, and reflect the caching behavior and associated TTLs. What is a plan for that?
I do believe Emily will be working on documenting this as part of the custom container next quarter and I can also help.

Would customizing the TTL or adding a no-cache option make sense? We are using default 2 weeks TTL, right? See: https://cloud.google.com/cloud-build/docs/kaniko-cache#configuring_the_cache_expiration_time.
I think default value makes sense, I didn't want to provide too many knobs to users since it may become more confusing or rarely used, but we can always provide additional flags for more advanced user to control it.

Thanks. It seems that caching may improve the startup time, and be useful for users who frequently launch the same pipeline. However I think caching may result in a difference in behavior. Questions:

Is it possible that caching will result in a stale image that users will perceive as undesirable and the behavior will be difficult to debug to users or support folks? For example, if a user pipeline depends on a latest version of a dependency X in pypi. Perhaps a dependency they control. They have a pipeline with a setup.py that has an open install_requires bound dep>=1.0.0 < 2. They run the pipeline, then push dependency to pypi and run the pipeline again, expecting a change in behavior. Kaniko will not rebuild the image in this case, right? What are your thoughts on that?

I think kaniko cache works the same way as docker layer cache, that is to say, if the locally downloaded artifacts changed(or requirements.txt, setup.py changed) it will actually change the COPY step in the prebuilding workflow. There will be no valid cache layer since the artifacts copy step and a new image will be rebuilt. (also verified through my own experiment with changing requirements.txt)

Thanks for checking. Sounds similar to Docker build cache https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#leverage-build-cache.
Transitive dependencies not present in requirements.txt may not be updated, but it would be better to list them in requirements.txt anyway to avoid pickling mismatches on the worker, as mentioned in https://beam.apache.org/documentation/sdks/python-pipeline-dependencies/.

During runtime with prebuilding workflow enabled, how visible is it to the user that the cached layers are reused and not rebuilt?

There will be log entries "No cached layer found for cmd ..." in the cloud build log.

Would it be possible to mention that caching is used when it is rather when it is not? Or perhaps add a  generic info message along the lines of: Staging pipeline dependencies into a prebuilt container image. To optimize build time, build steps will be cached.
Also do you know if a user can tell Kaniko to clean the cache manually?

I think we should document the prebuilding feature in Beam docs, and reflect the caching behavior and associated TTLs. What is a plan for that?

I do believe Emily will be working on documenting this as part of the custom container next quarter and I can also help.

Would customizing the TTL or adding a no-cache option make sense? We are using default 2 weeks TTL, right? See: cloud.google.com/cloud-build/docs/kaniko-cache#configuring_the_cache_expiration_time.

I think default value makes sense, I didn't want to provide too many knobs to users since it may become more confusing or rarely used, but we can always provide additional flags for more advanced user to control it.

Would it be possible to mention that caching is used when it is rather when it is not? Or perhaps add a generic info message along the lines of: Staging pipeline dependencies into a prebuilt container image. To optimize build time, build steps will be cached.
Also do you know if a user can tell Kaniko to clean the cache manually?

Whether cache is used in the docker build will be apparent in the build log and I think it will be more accurate than what we can log in beam workflow, see:

use cache: https://pantheon.corp.google.com/cloud-build/builds/8ee579d4-aa86-4c40-bb98-59d65160e223?project=beam-cloud-build-test
no cache layer found: https://pantheon.corp.google.com/cloud-build/builds/89f57168-b6e5-4327-a574-82e0767b4600?project=beam-cloud-build-test

I don't know if user can tell directly clean the cache but caching can always be avoided if user specifies a more specific docker registry push url. (the caching scoped to push url for example if user pushes to gcr.io/project/sdk_container_version_1/beam_python_prebuilt_sdk:tag, the cache will only exist if user build container with the same gcr.io/project/sdk_container_version_1 docker_registry_push_url)

Whether cache is used in the docker build will be apparent in the build log.
Agreed. However I currently build log shows only on error path. How about: we move the log link above? For example:

Building SDK container image with Google Cloud Build: (url). This may take a few minutes.

Whether cache is used in the docker build will be apparent in the build log.

Agreed. However I currently build log shows only on error path. How about: we move the log link above? For example:
Building SDK container image with Google Cloud Build: (url).
This may take a few minutes.

Sounds good, I've moved the logging before operation finishes.

Please fix YAPF. Thanks.

Thanks a lot, @y1chi !


Debug point statement

Would be neat to have a "stop execution here" button.

Also, step by step exec.
Stop before every statement


Thresholds for alerting

For example on the performance pane set a threshold above which you get a system notification

Alerting is probably not going to be implemented in Halin because the data is only alive for the scope of how long your browser is open.  Halin is not intended to be a replacement for services like datadog, stackdriver, etc.


json-reporting: add callsites dictionary to output

We have callsites for each function with respect to what functions they are calling. However, it's currently not output to summary.json. This fixes it.
Ref: https://github.com/ossf/fuzz-introspector/issues/1343

CFLite issues are false positives.


Death image at README.md

Nothing critical; The Image that is embedded in the README.md is not longer available.

Thanks, fixed.


Mac build overhaul

Currently, in Vim, there are two different classes of configuration for macOS.
One is with --enable-darwin (default) and the other with --disable-darwin.
The former takes advantage of the API specific to macOS, thereby enabling system clipboard integration and character set conversion of which encoding is specific to that OS.  The latter is identical with what is commonly used for other UNIX-like platforms.
When "the darwin feature of Vim" (h: mac-darwin-feature) was merged into our code base, it appears that some adjustments were done incompletely for some reason.
As a result, for non-darwin Vim, has('mac') returns 0, and :version doesn't displays anything which is compared to "MacOS X (unix) version" for darwin Vim.
That makes it hard to write Vim scripts for non-darwin Vim.
That's not the only issue.  Even for the darwin configuration/build, the configure script refers to the feature as MACOS, the compiler command-line calls it MACOS_X_UNIX, a header file gives it two different nicknames, MACOS_X and MACOS, and they are used in several *.c files in an obscure way.  That's anything but manageable.
Also, despite the fact that the support for Mac OS Classic has been discontinued since Vim 7, there are quite a few remnants of it in our code base.  Obviously, that gives unnecessary overhead to code reading and maintenance.
One could look away the last two issues and write a quite simple patch to fix the primary issue.  Actually, I did that.  But that had made me uneasy because I knew such a way of solution would only get the situation worse and get back at me someday, even if it could be helpful for the moment.
My new solution is to examine every use of the MACOS.* macros, determine what each is meant for, and get the build system neat and tidy on the basis of that research.  This is not an attempt to change the build system in favor of someone's view or preferences, but an attempt to give it a better shape so that it will clarify itself, what each part of it does and how each relates to other.  I'd like to call it a refactoring, even if it happens to include substantial changes such as bug fixes as "side effects" of it.
To ease the review, the whole patch is logically divided into several commits (ie., it's not a chronological accumulation), and each commit addresses its own issue only.  Honestly, I'm not sure how much it could help people review the patch, but both the whole of it and each of the commits constituting it are easily available to them on GitHub (AFAIK), I try using it.
The patch consists of about 20 commits.  The first three commits will clarify the goal.  The three commits following them fix some breakage in the build system resulted from them.  Then many minor adjustments follow.  The last two commits clean up what I think is the root cause of the headache.  Please refer to the commit messages for details.
Sorry for the wall of text and thank you for taking your time.  I welcome any comment on the proposed patch and open to any proposal.

Codecov Report

Merging #2178 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2178      +/-   ##
==========================================
+ Coverage    74.2%   74.22%   +0.02%
==========================================
  Files          90       90
  Lines      131958   131959       +1
  Branches    30860    28980    -1880
==========================================
+ Hits        97913    97946      +33
+ Misses      34016    34013       -3
+ Partials       29        0      -29

Impacted Files
Coverage Δ

src/main.c
55.12% <ø> (ø)
:arrow_up:

src/misc1.c
82.55% <ø> (ø)
:arrow_up:

src/if_python.c
77.43% <ø> (ø)
:arrow_up:

src/undo.c
81.76% <ø> (ø)
:arrow_up:

src/evalfunc.c
83.34% <ø> (ø)
:arrow_up:

src/term.c
51.23% <ø> (ø)
:arrow_up:

src/if_ruby.c
56.46% <ø> (ø)
:arrow_up:

src/ui.c
45.76% <ø> (ø)
:arrow_up:

src/fileio.c
74.86% <ø> (ø)
:arrow_up:

src/version.c
79.29% <ø> (-1.02%)
:arrow_down:

... and 17 more

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 0aa398f...fdab4b0. Read the comment docs.

2017-10-02 0:48 GMT+09:00 chdiza<EMAIL_ADDRESS>
@chdiza commented on this pull request.
In src/vim.h https://github.com/vim/vim/pull/2178#discussion_r142033099:

@@ -85,9 +85,15 @@
#endif

/*

MACOS_X	    compiling for MacOS X

MACOS_X_DARWIN   compiling with the darwin feature for MacOS X

MACOS	    compiling for either one

MACOS	    compiling for any of Macintosh operating systems

I wonder what this is for, now that Classic has been ripped out. There are
now no Macintosh operating systems supported by Vim other than "MACOS_X".

Thank you for the comment and good question.  I also thought that was one
of the things we should get rid of.
However,when I just started carrying that out, I found that the MACOS
blocks are closely related to the Command keys.  They are not specific to
Mac OS X but rather something identifying the Mac hardware.  This is the
very reason why I didn't purge MACOS.
If the comment for it is misleading or confusing, I'm open to changing
that.  Maybe, "hardware" instead of "operating systems"?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/vim/vim/pull/2178#pullrequestreview-66340068, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ANL7Z9HMtSoACTiucN_hnhPgrob1M5HIks5sn7RIgaJpZM4Pp0mc
.

Having replied to @chdiza , just to be sure for myself, I looked over all the places where MACOSs are used this morning, and found that some of them still looked fishy.
For example, we have two MACOSs in misc2.c:newenv().  But if we combine that fact with the declaration of environ in misc2.c, taking it into account that Mac OS X has  the system global environ(7)  (Not sure about its early versions, though), we see that those two MACOSs are a wrong substitute for MACOS_CLASSIC.
Probably, they have been sitting there since MACOS_CLASSIC was introduced, and overlooked because they are obviously harmless for Vim to work properly.  But..., yeah,  I'll look into this new issue and update the PR if necessary.  Maybe, after making sure that such a misuse is completely expelled from the code base, I'll replace MACOS with MACOS_X, because the former can be interpreted as either "Mac OS" or "macOS", or both.  Eliminating this sort of ambiguity is within the scope of the PR, anyway.

I'll look into this new issue and update the PR if necessary.

Done with the last six commits.

I'll replace MACOS with MACOS_X, ...

Will be done tomorrow.

I am unclear as to why a build of a console-only app is now linked to the AppKit framework.  The system vim that comes with macOS doesn't have this.

I am unclear as to why a build of a console-only app is now linked to the AppKit framework.

Before the inclusion of the patch, that link had been done against Cocoa, and this implies that the app is also linked against AppKit; the linking is done by the linker at build time implicitly to resolve Cocoa's dependency automatically.
For Vim to support the darwin feature, in particular, system clipboard integration, it needs NSPasteboard of AppKit and hence has always been linked against it when the feature is included.

The system vim that comes with macOS doesn't have this.

This is because, since it doesn't support system clipboard integration, there's no need for AppKi or Cocoa for it.
I've never heard of any reason why Apple avoids the feature, but one thing sure to me is, if one runs Mac without GUI (i.e., console mode) and invokes a vim which is linked against any of GUI frameworks such as AppKIt or Cocoa, launchd will probably start up the windowing system and prevent him from keep working in console mode.

By "console-only", I meant in-terminal-only, not the special "console" mode in which Aqua isn't even running.
But thanks for the explanation, that makes sense.


[Networking] Use ClusterIP when possible

Context
Today, Compose on Kubernetes creates Headless Services to enable Service to Service communication. This
brings the same developer experience as when working with docker-compose or swarm, but it is not always the best option. We should prefer creating ClusterIP if we get enough information about how to configure it properly.
Headless service pros

No KubeProxy involved: direct POD to POD communication means less latency, less load on the kube-proxy component
Do not require a list of exposed port: As there is no proxy involved, a headless service alows to reach any port opened by a POD

Headless service cons

No real load balancing: the DNS server only answers with as many records as there are PODs matching its selector. It is the responsibility of the client network stack to decide if it should do some round-robin on those DNS entries. Many development runtime don't do that, and worse than that, some just take the first entry into account
Scalability issue: depending on DNS caching policy on the client, its knowledge of the valid PODs IP can be stale. Worse, some network client stacks have a very aggressive DNS caching policy, and never re-query the DNS server: those can become stuck trying to reach PODs that have been unscheduled due to a change in service scaling.

Potential solution
Introduce the option to use ClusterIP services instead of headless using this strategy:

introduce a field on ServiceConfig named InternalServiceType that can be Headless or ClusterIP
introduce a default behavior, that looks for the Expose field in ServiceConfig (see https://docs.docker.com/compose/compose-file/#expose): if it is empty, defaults to Headless. Else use ClusterIP

ClusterIP ports configuration will use the Expose field in the ServiceConfig to decide which ports to route.

I looked at the PR and it seems there are 2 new properties can be set InternalPorts and InternalServiceType. How do I go about using these as they are not valid compose file property?

Hello @phng , this feature was introduced in the docker/cli with this PR https://github.com/docker/cli/pull/1815/files#diff-b3114871e359bbcd13674fefe7b3e16eR30 and then refactored on this PR https://github.com/docker/cli/pull/1823 .
To summarize, you can use this feature in your compose file adding
services:
  test:
    image: "some-image"
    x-kubernetes:
      internal-service-type: {"Headless"|"ClusterIP"}

This feature is available with the latest 19.03 docker cli (should be shortly released), and is experimental (as there is a x- prefix). We definitely need some feedbacks about this feature so please try it 👍

@silvin-lubecki Thanks for the instructions.
I added
mdm:
    image: mdm
    build:
      context: ../server
      dockerfile: Dockerfile
    env_file:
      - mdm.env
    deploy:
      replicas: 2
    x-kubernetes:
      internal-service-type: ClusterIP

However, the service created is still a headless headless service.
kubectl describe service mdm
Name:              mdm
Namespace:         default
Labels:            com.docker.service.id=rrts-mdm
                   com.docker.service.name=mdm
                   com.docker.stack.namespace=rrts
Annotations:       <none>
Selector:          com.docker.service.id=rrts-mdm,com.docker.service.name=mdm,com.docker.stack.namespace=rrts
Type:              ClusterIP
IP:                None
Port:              headless  55555/TCP
TargetPort:        55555/TCP
Endpoints:         <IP_ADDRESS>:55555,<IP_ADDRESS>:55555
Session Affinity:  None
Events:            <none>

It looks like my Docker Desktop kubernetes version is 1.14.3, and stack api version is v1beta2. How do I upgrade the server version to v1alpha3? Or do I have to wait for the new release in edge channel?


Augment Highlighted Sites

Improve how highlighted sites are grouped. One idea is have 2 pages, one to select host+protocol and the second to select sites belonging to that host

Fulfilled with PRs below:
https://github.com/ChrisERo/vocab-forager/pull/38
https://github.com/ChrisERo/vocab-forager/pull/37


finder window too small on large screen

Description

The finder popup is too small on my screen, it also will only show vertically stacked as below:

Expected Behavior

Horizontal stacking like in the readme.
Can we please add a preference option for this window sizing?
Actual Behavior

As pictured
Neovim Built in Behavior

Details

no repro sorry

Environment

nvim --version output: ```:!nvim --version
NVIM v0.5.0-dev+1281-gecf075eb2
Build type: RelWithDebInfo
LuaJIT 2.1.0-beta3
Compilation: /usr/bin/cc -O2 -g -Og -g -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wmissing-prototypes -Wimplicit-fallthrough -Wvla -fstack-protector-strong -fno-common -fdiagnostics-color=always -DINCLUDE_GENERATED_DECLARATIONS -D_GNU_SOURCE -DNVIM_MSGPACK_HA
S_FLOAT32 -DNVIM_UNIBI_HAS_VAR_FROM -DMIN_LOG_LEVEL=3 -I/home/nicky/apps/neovim/build/config -I/home/nicky/apps/neovim/src -I/home/nicky/apps/neovim/.deps/usr/include -I/usr/include -I/home/nicky/apps/neovim/build/src/nvim/auto -I/home/nicky/apps/neovim/build/include
Compiled by nicky@nt-arch-sfc

Features: +acl +iconv +tui```

Operating system: Arch Linux
lspsaga commit: cb0e35d2e594ff7a9c408d2e382945d56336c040

I also experience this issue.

I have this same problem as well.

I have same problem as well, temp solved by modify two files

lspsaga/window.lua
lspsaga/provider.lua

and here is the diff
diff --git a/lua/lspsaga/provider.lua b/lua/lspsaga/provider.lua
index 22c0d86..66b2daf 100644
--- a/lua/lspsaga/provider.lua
+++ b/lua/lspsaga/provider.lua
@@ -322,7 +322,7 @@ function Finder:auto_open_preview()
       opts.row = finder_win_opts.row + height + 2
       opts.col = finder_win_opts.col
       opts.width = min_width
-      opts.height = 8
+      opts.height = 40
       if self.WIN_HEIGHT - height - opts.row < 4 then
         return
       end

diff --git a/lua/lspsaga/window.lua b/lua/lspsaga/window.lua
index 017cb00..e5d733c 100644
--- a/lua/lspsaga/window.lua
+++ b/lua/lspsaga/window.lua
@@ -54,7 +54,7 @@ local function make_floating_popup_options(width, height, opts)
   local new_option = {}

   new_option.style = 'minimal'
-  new_option.width = width
+  new_option.width = 120
   new_option.height = height

   if opts.relative ~= nil then

I have same problem as well, temp solved by modify two files
* lspsaga/window.lua

* lspsaga/provider.lua

and here is the diff
diff --git a/lua/lspsaga/provider.lua b/lua/lspsaga/provider.lua
index 22c0d86..66b2daf 100644
--- a/lua/lspsaga/provider.lua
+++ b/lua/lspsaga/provider.lua
@@ -322,7 +322,7 @@ function Finder:auto_open_preview()
       opts.row = finder_win_opts.row + height + 2
       opts.col = finder_win_opts.col
       opts.width = min_width
-      opts.height = 8
+      opts.height = 40
       if self.WIN_HEIGHT - height - opts.row < 4 then
         return
       end

diff --git a/lua/lspsaga/window.lua b/lua/lspsaga/window.lua
index 017cb00..e5d733c 100644
--- a/lua/lspsaga/window.lua
+++ b/lua/lspsaga/window.lua
@@ -54,7 +54,7 @@ local function make_floating_popup_options(width, height, opts)
   local new_option = {}

   new_option.style = 'minimal'
-  new_option.width = width
+  new_option.width = 120
   new_option.height = height

   if opts.relative ~= nil then

great! I toned it down a little bit but now feels way better, thanks for the tip!


[$1000] User can't download the production app from playstore

If you haven’t already, check out our contributing guidelines for onboarding and email<EMAIL_ADDRESS>to request to join our Slack channel!

Action Performed:

Go to playstore
Download new expensify
Open the app and login
Check if it has a stg badge at top

Expected Result:
The app should not have the stg badge and stg features
Actual Result:
The app is stg
Workaround:
unknown
Platform:

Where is this issue occurring?

Android

Version Number:
Reproducible in staging?: need repro
Reproducible in production?: need repro
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Upwork job URL: https://www.upwork.com/jobs/~0138615bf32be2c02c
Issue reported by: @Puneet-here
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1662562500746549?thread_ts=1660710162.573049&cid=C01GTK53T8Q
View all open jobs on GitHub

@mvtglobally are you able to reproduce?

@mallenexpensify @mallenexpensify I was checking with the team on the same. There is a tricky way to download PROD build on Android with the same device as you have staging. Currently we all see STG
Since all our testers are already getting BETA (STG) builds, not sure this result is accurate.
Is there any chance you can ask someone in Expensify internally who DOES NOT have staging build to download PROD build?
https://user-images.githubusercontent.com/43995119/196693331-402beb99-5498-4497-b2a6-ca15d1eaeb11.mp4

Asking internally https://expensify.slack.com/archives/C01SKUP7QR0/p1666291488048139

Gonna close cuz @alex-mechler was able to help test and it looks like its not repro

@mallenexpensify, in the screenshot I can see tha stg badge at the login page.

ah shit... you're right @Puneet-here , great 👀 .
Reopening and assigning an engineer to take a look

Oh, I think this is a dupe of https://github.com/Expensify/Expensify/issues/237206 and it is probably internal

Oh wait, maybe not. Forget that.

Ummm, isn't play store downloading the staging app because you are in the beta???

Ummm, isn't play store downloading the staging app because you are in the beta???

No, I don't think this is the case because external contributors aren't in the beta and I have tried some different accounts that I've never used at expensify but I still couldn't download the production app.

Job posted https://www.upwork.com/jobs/~0138615bf32be2c02c

Hi
I saw the job on Upwork and did some digging, checked the code and everything seems to be good, unless someone pushed a staging build to the production?
Coz the environment variables are the only things that differentiates things in the process, so maybe the build pushed to the production (public) is a staging environment build?
If not, can I more context on it?
I mean like access to things like developer console, from which I can find answers and most probably solution?

did some digging, checked the code and everything seems to be good, unless someone pushed a staging build to the production?

I also feel the same. @srikarparsi, can you please look into it?

I am not too sure but seems to me that it could be a caching issue with what build gets downloaded and then that build does not match with version in package.json in the build. And then we see a badge. I hypothesize this as I see version shown in screenshot is different from what should have been in playstore at that time ( by looking at the release in github). We have new build versions deployed to PROD when this code was being discussed. See the below comment.

I could be totally wrong in my many assumptions. Silly question with that,
Why the present android version in playstore is 1.2.16-5 while the github release says it should be 1.2.17-4

I don't believe there was an accidental deploy, as our internal releases line up with the PlayStore builds. Instead, it seems like a regression in our versioning logic as was suggested above.
Staging version: 1.2.18
Prod version: 1.2.19-1

that means there would have to be a PR recently which would break this logic, that could help narrowing this down

@mallenexpensify, in the screenshot I can see the stg badge at the login page.

Also, the version is definitely incorrect in that screenshot.

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

Adding the deployBlockerLabel  to prevent further deploys until we can confirm the cause. I know it's useless given this exists on prod, but it'll hopefully speed up the release of the evental fix 🤞

My current guess is that the bug is in our CI deploy logic, as presumably Applause would have noticed the incorrect version during testing -- or would have reported bugs against a clearly outdated version

that means there would have to be a PR recently which would break this logic, that could help narrowing this down

Just want to inform that I had reported this on Aug 17, if it helps.

This is the deploy checklist for the current prod build, which occurred on August 17th. There were two releases in quick succession on Aug 16th too (1 + 2).
Nothing stands out as a potential cause 😕

Removing deploy blocker label since its not really a blocker. But keeping the hourly tag since it sounds urgent to address. I think we can also create a fire-room to discuss this.

Any update here @chiragsalian ?  Asking cuz it's hourly and I'm rarely assigned to hourly issues

No idea, i haven't investigated this issue since I thought others were investigating. Could either @Julesssss, @Puneet-here, or @thesahindia summarize where we currently stand on the issue and if there are any next steps to tackle this?

@chiragsalian I wasn't assigned either, just tried to help investigate when I saw it raised. As it's no longer a blocker I think we just need to retest on later releases to see if the staging icon is still shown.
@Expensify/applauseleads are you still able to reproduce this?

This is still reproducible but not sure what the problem is.

Does this still need to be Hourly?
Why hasn't it gone overdue if it's Hourly?

@chiragsalian or @Julesssss do you guys have the bandwith to take over this issue? I'm assigned currently but don't know enough about deploys to troubleshoot. I'll shadow whoever takes it over though so I can learn a little about the process.

do you guys have the bandwith to take over this issue? I'm assigned currently but don't know enough about deploys to troubleshoot.

I'm afraid not, but also I no longer believe this was a deploy issue. Pretty sure there has been a regression at some point

Doubled price to $500
https://www.upwork.com/jobs/~0138615bf32be2c02c
@michaelhaxhiu Reassigning as I'm heading OOO for a month

Ok I've read through this thread. @srikarparsi if you are looking to pass this off, can you post in a slack channel and link here? Let's get this handed off today if we can to keep progress on the daily nature.

sounds good, thanks @michaelhaxhiu. I just posted here

The verdict from the thread that @srikarparsi started is that this will remain on [hold] until the following is finalized:

Maybe not dupe, but similar in the sense that env doesn't exactly work the same on native. So once this is fixed via this PR: https://github.com/Expensify/App/pull/12546
It should be easier to investigate or fix

Maybe not dupe, but similar in the sense that env doesn't exactly work the same on native. So once this is fixed via this PR: #12546. It should be easier to investigate or fix

The environment value that set the badge to STG, is already being computed differently for native.  Which is by comparing pkg.version in the build with android play store set version. Only when both the version are set different, we shall see the STG badge. We must think if there is a process deficiency during prod release or any other manual error that can cause such a thing !

This is on Hold, but I'm re-assigning as I'm going OOO. PR is linked above that's blocking this, I have a feeling it could finish sometime next week @maddylewis

The verdict from the thread that @srikarparsi started is that this will remain on [hold] until the following is finalized:
Maybe not dupe, but similar in the sense that env doesn't exactly work the same on native. So once this is fixed via this PR: https://github.com/Expensify/App/pull/12546
It should be easier to investigate or fix

moving to weekly until this is off hold.

@srikarparsi - could you confirm the details in @tjferriss' comment - https://github.com/Expensify/App/issues/11132#issuecomment-1325483442?
thanks!

hey sorry yeah I was waiting on the hold issue to complete before responding. I can take this on internally so I will add the internal label :)

I was looking into it today and it seems like the value that points to the staging API just needs to be set to false which will be done in this PR. @mountiny could you confirm? If so, I think we can close this issue.

I am not sure if that PR will resolve this. The PR is mainly looking at the API requests. Here the issue is the Staging badge, which in native apps has different logic that which is touched in the aforementioned PR. We use this getEnvironment method which in android uses this betachecker api request to find out if we match the staging App store version or not.
So I would look at that file/ code and see if by any chance something changed with the version logic there.

@srikarparsi have you had a chance to look into this?

So I would look at that file/ code and see if by any chance something changed with the version logic there.

hey @tjferriss! I haven't had a chance to look into it yet but I'm looking into it now.

Awesome, @srikarparsi. This is now one of the oldest bugs in the repo, so it would be great to keep the pace on it. Thanks!

Talking to vit about this but believe that the issue is from line 14 in this PR

@maddylewis taking this back, it was assigned to me before I went OOO.
Let's keep this daily since it sounds like @srikarparsi and @mountiny found the root cause.

asked for example of play store url text here to help me write regex.

Discussing a more long term solution here

Unassigning myself as andrew is working on the fix

PR sent 📤

This will hit the 'one week after deployed to production' milestone tomorrow.  It looks like the only person dude compensation is @Puneet-here for reporting.  If I'm missing someone, please comment

Asked in the PR if we had closed to loop on the ProductionQA actually.

@mallenexpensify Sounds good 👍 - Testing passed

@Puneet-here can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~019d52081ca835b9ef

Accepted @mallenexpensify

@Puneet-here paid $250 for reporting
@AndrewGable we want to update regression test steps for this, right?   If so I'll leave open and tackle the last piece.
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] The PR that introduced the bug has been identified. Link to the PR:
[ ] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

Yep - We can add a regression test for this.

Posted regression test steps in #bug-zero
https://expensify.slack.com/archives/C01SKUP7QR0/p1672711388472929

Likely running with this for regression test steps, will create the GH for tomorrow

Ensure App isn't loaded on device
Click Android on an Android device
Follow steps to download app
Verify app has downloaded to device
Verify the app does not way staging on the login page.

I put 'does not' cuz that should be the expected behavior and staging should be the edge or test case, right?  I'm still new to these steps, I'm assuming we're always creating them for the production version of the app, let me know if that's not correct

Issue created to update test steps https://github.com/Expensify/Expensify/issues/253420
Are these needed before we close this out @AndrewGable ?

The PR that introduced the bug has been identified. Link to the PR:
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

I do not think this is required, the code relied on an external site that we do not have control over, so it actually started to fail without a code change on our side.

Thanks @AndrewGable.
@thesahindia are you able to complete the above steps?  I think the process we're working towards is to have C+ complete all items they can. For any that are related to backend changes, the C+ would comment on the issue, tag the assigned internal engineer, and provide reasoning why they're unable to complete the steps.

I don't have much knowledge on this issue since I didn't do anything here. Just a heads-up that @srikarparsi mentioned that it was a regression from https://github.com/Expensify/App/pull/9960

I think as Andrew mentioned, @mallenexpensify we should be good to close this one out, seems like the Google API response has changed and that introduced this problem. We could not do much else other than have a regression test which will check for this.

K, closing.  Thanks @thesahindia and @mountiny


I am getting axios.create is not a function Error

Maybe somebody else has seen this, hope somebody has some advice
import Replicate from "replicate";

const replicateToken = process.env.REPLICATE_API_TOKEN || "";

const replicate = new Replicate({
  auth: replicateToken,
});

const imageToText = async (imageFile: File) => {
  const output = await replicate.run(
    "methexis-inc/img2prompt:50adaf2d3ad20a6f911a8a9e3ccf777b263b8596fbd2c8fc26e8888f8a0edbb5",
    {
      input: {
        image: imageFile,
      },
    }
  );
  return output;
};

export default imageToText;

I am using a freshly minted CRA application

@WernerWildenboer What version of the replicate package are you using? We just removed axios as a dependency yesterday with version 0.9.0.

@WernerWildenboer Sorry, I just noticed you're mentioning that this was for a CRA application. This library is for Node.js applications and doesn't currently support client-side JavaScript. If you want to build a website with the Replicate API, check out our guide for building a website with Next.js.

Ah I am with you; awesome. Thank you so so much!


Bug on Windows, NodeJS POSIX vs. Windows paths

Tried it on Windows and building fails because of a paths issue. On Windows all the item paths in the resources folder are written as
[
  "D:\\path\\to\\my\\project\\resources\\css\\page.css",
  // etc.
]

I think utils.js > walk #L10 needs to be
            let itemPath = path.posix.join(dir, item);

This seems to make walk() produce the correct resources.json file:
[
  "resources/css/page.css",
  // etc.
]]

Luckily this problem is literally the first section in the NodeJS path documentation so I was able to figure it out. :smile:
Thank you!

Oh thanks for looking into it, I don't use windows so this tool that I initially just wrote for myself is probably inadvertently biased. The next time I make any updates I'll have a look including at the fix you mention.


Loading of Splitview state from json adds extra div containers that break layout

Describe the bug
api.fromJSON loads state of Splitview, but also adds two empty split-view-container divs
To Reproduce
Codesandbox with reproduction. Scroll preview up and you'll find bunch of extra empty space from that unexpected divs. Duplicating api.fromJSON call leads to increased number of empty split-view-container divs
Expected behavior
Extra empty divs are not added to markup
Desktop:

Browser Firefox
Version 129.0.2

In 2.0.0 still same issue with extra divs with .dv-split-view-container class.
CSS workaround if anybody interested:
/* bug https://github.com/mathuo/dockview/issues/721 workaround */
.your-splitview-class > div.dv-split-view-container:not(:last-child) {
  display: none;
}

Could not be suitable if state restoration is done very often as divs still remain in DOM and can cause memory issues.


Device Types Height Field

Environment

Python version:  3.5.2
NetBox version:  2.4.3

Proposed Functionality
When adding devices, is it possible to enable non-integer RU values?  I'm adding in my estate and my c6524ME boxes are 1.5RU.  Not a big deal, but it would be nice for me at least and maybe others.

Use Case
Put devices into inventory that aren't integer values of RUs.

Database Changes
I don't know enough to answer this or the external dependency question....

External Dependencies
??

Please see #51


Netflix

System Details

OS: [ Windows 11 Pro]
Thorium Version [117.0.5938.157 (Official Build) (64-bit)]

Problem
Not able to play any of Netflix. I have tryed clean and still not netflix. I know its a setting not sure what setting it is. Disney, Hulu and paramountplus, Play fine. Just Netflix is my problem child here.
Screenshots
If applicable, add screenshots to help explain your problem.
Additional Notes
Any steps to try open to try. I have gone as far as reinstalling it with not a thing and still problem.

In Youtube also some videos arent possible to be played because Thorium is too old :(

@blindmodz Can you provide an URL? Thanks!

@blindmodz Can you provide an URL? Thanks!

https://www.youtube.com/watch?v=Be5ssrg5xws (might be my account because if i open it on incognito mode works fine)

@blindmodz Can you provide an URL? Thanks!

https://www.youtube.com/watch?v=Be5ssrg5xws (might be my account because if i open it on incognito mode works fine)

Hmm, seems to work fine on my side. I also checked by using "Open New Window with a temporary profile" and it works.
Maybe you have some chrome extensions that are causing the issue.

Prime Video has issues as well - namely a banner saying that HD video can't be played as the browser is incompatible.

This may also have something to do with some codecs and some websites. Not all these checks always
seem to be true. For instance, on certain websites - you know the educational ones about girls - I may
insta-skip to the middle of the video, and it will tell me that my browser can not play this video. But
then I refresh the page, and simply click on the play button, then it begins to play, and THEN I can
jump to e. g. the middle of the video and it plays fine. Makes no real sense to me, other than something
in the video-check being awkward. Speaking of which, I think thorium should get bundled with a
default but simple video downloader too, such as yt-tdlp or what was the name again.

There are also other video platforms that have the same issue such as F1 tv. I have to open Chrome to watch either F1 or Netflix.

adding

https://github.com/Alex313031/thorium/blob/main/docs/FAQ.md


Add support for pyi files

Fixes #207. This commit applies the format to all of typeshed: https://github.com/JelleZijlstra/typeshed/commit/0fe45a0726362f63084ad7714bc819d7bd6a3eb1.
One thing I haven't done yet is removing blank lines between classes with trivial bodies (see e.g. here: https://github.com/JelleZijlstra/typeshed/commit/0fe45a0726362f63084ad7714bc819d7bd6a3eb1#diff-fc608da07debda33805668ed07dd237eL249).

Pull Request Test Coverage Report for Build 356

53 of 60 (88.33%)  changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 95.287%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

black.py
46
53
86.79%

Totals

Change from base Build 355:
-0.2%

Covered Lines:
2022

Relevant Lines:
2122

💛  - Coveralls

This is very cool. Thanks! ✨ 🍰 ✨


Use test-drive directly instead of vendoring it

Staging for https://github.com/fortran-lang/stdlib/pull/494
Requires https://github.com/fortran-lang/test-drive/pull/6
cc @milancurcic

Great, thank you, let's do it. Do you know why the macOS tests for sleep are failing? Earlier when I worked on https://github.com/fortran-lang/stdlib/pull/494 it seemed like they were transient (re-running jobs made some of them go away, I think). Anybody with a macOS that could check this locally?


Consider running Envoy through CNI instead of as sidecar

Running Envoy as a sidecar is not the proper way to set up a pod's networking interface.
Since there's no way to specify that the Envoy container must be fully started before the pod's main container starts, the main container might see failed connection attempts (see https://github.com/istio/istio/issues/11130). The same happens during shutdown if Envoy shuts down before the main container (https://github.com/istio/istio/issues/7136).
When using istio-cni, the app's init containers aren't able to access the network (https://github.com/istio/cni/issues/77), since the iptables rules are already set up, but the Envoy proxy isn't started yet (as a sidecar, it will only start when all the init containers have terminated).
When you're not using istio-cni, the app's init containers can talk to whatever service they want, since the iptables rules aren't set up yet (Istio's init container is added last, right?).
Also, as soon as you try to use Istio in a properly secured Kubernetes cluster, you run into the problem, where your application pods fail to deploy, because Istio is trying to inject the sidecar, and the sidecar fails PodSecurityPolicy admission controller's validation (also happens with other admission controllers, for example: https://github.com/kubernetes/kubernetes/issues/64333). This happens because during the creation of the Pod, the PSP controller gets called first (it mutates the Pod based on PSP that applies to the pod), then Istio's sidecar injector adds an additional container, and then the PSP controller gets called again (this time it's not allowed to mutate the pod, but only validate it). Since the sidecar container may not match the PSP that applies to the pod, the pod gets rejected. Currently, there's no proper way to fix this.
All of the problems described above point to the fact that running Envoy as a sidecar is not the proper way of augmenting a pod's networking interface. But, luckily, there's CNI. Instead of having istio-cni set up just the iptables rules, it should also start the Envoy proxy.
I'm not suggesting to run only one Envoy proxy for the whole node (due to obvious security concerns). I'm suggesting to keep one proxy per pod, but instead of running it as a regular pod container, we spin it up in the istio-cni plugin itself.
Pros:

does't require any changes to Kubernetes (no need for container or mutating webhook ordering)
properly secures the network for all of the pod's containers (including init containers)
doesn't pollute the pod with an additional container

Cons:

won't be able to access Envoy's logs as easily as now
won't be able to check status of the sidecar as easily as now
won't have the automatic restart features provided by k8s liveness probes (this would have to be handled by istio-cni)

FYI: I've started implementing this. I expect to have a very crude proof-of-concept implementation some time next week.

@luksa When you have an implementation we will need to work out how to get to the access logs and status information. Let me know if you have any thoughts on this.

Status update: I managed to get the first version of the PoC working last week. Since the CNI plugin is calling docker directly, it only works on clusters that use Docker as the container runtime (and a specific version of Docker is required, since I'm just calling the docker binary instead of using Docker's API).
I'm now modifying the code so it uses the CRI interface, just like the Kubelet. Actually, I'll need to replicate some features that the Kubelet provides:

keeping the cert secret volume in sync with the k8s Secret
automatic restart of container

I'll try to import the existing Kubelet code for this.
@liamawhite The status information is already accessible without any problems - the proxy is running in the pod's network namespace, so you can access the proxy's open ports externally. For the access logs, I was thinking we could possibly have users add a sidecar that would get the logs from Envoy and just forward them to its own stdout/stderr.
So, the PoC has proven that this approach can work, but it has made me realize one big problem with all this: namely, computation resource requests, limits and quotas.
Since the proxy is no longer a proper sidecar (it isn't defined in the pod's manifest), kubernetes obviously doesn't know anything about it. The scheduler thus doesn't take into account the resource requirements of the proxy. And the actual resource usage of the proxy doesn't count towards the user's quota. Basically, the proxy becomes a system-level service.
For that reason, I'm not 100% sure that this is the correct approach.

I recall, in the networking wg meeting the cni plugin approach was first presented, there was an agreement that it should be possible to launch a sidecar from cni that kubernetes should be fully aware of. @costinm do you recall that meeting and who said it should be possible?

Hey, we here at Monzo have open sourced an alternative solution to this sequencing problem:
https://github.com/monzo/envoy-preflight
The idea is, it's a wrapper around your main application, which ensures it starts after envoy is live, and shuts down envoy when its done. You'll still need to prevent sigterms from reaching envoy.

Could we inject the wrapper by modifying the pod args ? It's an interesting idea.

Hello, I wonder whether if Envoy could run through CNI plugin instead of sidecar, how many Envoy proxy container are there in each node? For example, worker node has 2 pods, does each pod has 1 envoy container or 1 Envoy container serves many pod?

Each pod has its own envoy container, just like in the normal Istio behavior.
Anyway, I no longer think running the sidecar through CNI makes much sense. We've been able to fix all the issues and no longer need this hack. But, on the other hand, new Kubernetes features will make the idea of running a container that Kubernetes isn't aware of, much less hacky - I'm referring to pod runtime classes and pod overhead.

@luksa

We've been able to fix all the issues and no longer need this hack
can you point to the change that fixed it and describe how ? Also which version of istio did the fix go in ?

@kuberkaul Sorry, I should have been more specific. By "we" I mean the Istio team at Red Hat. Our changes are currently only in our Maistra repo, but should be pushed upstream eventually (we were pressed for time, so couldn't afford to wait for the changes to be integrated into upstream directly).
The changes that were required were mostly to remove the need for elevated privileges (istio components require running under a specific user ID - we reconfigured everything so that the randomly-assigned user ID that Openshift assigns to pods is used). We also modified the sidecar injection template to drop some Kernel capabilities that the SCC/PSP admission controller requires to be dropped (if you don't drop them, the admission controller rejected the pod due to admission controller ordering issues - these are now resolved in the latest Kubernetes version). More info is here: https://issues.jboss.org/browse/MAISTRA-549
We still need to solve the problem of the sidecar not running in the init phase of a pod (when its init containers run), but I'm hopeful the new Kubernetes sidecar proposal eventually provides a way to run init container sidecars.

@luksa Do you have any idea how we can run Envoy proxy through CRI?


Loss function

I find that you use nll_loss as the loss function in PGDtraining, why you don't use cross_entropy?
In ResNet, you use the cross_entropy to train the model

Whether the same effect can be achieved with these two loss functions？

You can refer to this link to see the difference of torch.nn.NLLLoss and torch.nn.CrossEntropyLoss : https://discuss.pytorch.org/t/difference-between-cross-entropy-loss-or-log-likelihood-loss/38816

also: https://pytorch.org/docs/stable/generated/torch.nn.CrossEntropyLoss.html

So I think that you should use cross_entropy  as the loss function in PGDtraining?
Because there is not log_softmax in your resnet, vgg, densenet and so on.

Yes, you are right:) There is a mismatch between the CNN model and other models. I would use CrossEntropyLoss instead of nll_loss and update CNN model to make it unified. Thank you for reporting this issue.


Run nyc with correct packager manager command

fixes #266
runs nyc command using the correct package manager exec command to fix compatibility with pnpm

Hey @yannbf
Are there any other blockers here? I'm running into this problem with pnpm.

Hey @yannbf Are there any other blockers here? I'm running into this problem with pnpm.

Hey Gil! This contribution does not account for certain monorepo settings because it doesn't find the files up, e.g. executing a script in a directory that has a package.json but not a .yarnrc file, which is higher in the tree. I'll try to improve this next week!

Thanks for the suggestion @watcher46!
I continued the work on this PR and I think it's done. However I'm creating another PR to allow for the canary release workflow to be done, which wasn't possible in the fork from this PR. Please follow that PR instead: https://github.com/storybookjs/test-runner/pull/364
For the ones interested, please try the canary and provide feedback on the other PR:
<EMAIL_ADDRESS>
Once I have proper feedback that this fixed your issues, I will cut a release.
Thank you!


Requires PIL/Pillow

You should specify that this game needs PIL/Pillow to run from source in the readme (like you specify pygame)

It doesn't require PIL. That was a part of fixing one of the bugs and I have gotten rid of it since.
-8BitToaster


[SPARK-38588][ML] Validate input dataset of ml.classification

What changes were proposed in this pull request?
1, add validation methods in DatasetUtils;
2, use the new methods to check the input dataset;
Why are the changes needed?
LinearSVC should fail fast if the input dataset contains invalid values:
import org.apache.spark.ml.feature._
import org.apache.spark.ml.linalg._
import org.apache.spark.ml.classification._
import org.apache.spark.ml.clustering._
val df = sc.parallelize(Seq(LabeledPoint(1.0, Vectors.dense(1.0, Double.NaN)), LabeledPoint(0.0, Vectors.dense(Double.PositiveInfinity, 2.0)))).toDF()

val svc = new LinearSVC()
val model = svc.fit(df)

scala> model.intercept
res0: Double = NaN

scala> model.coefficients
res1: org.apache.spark.ml.linalg.Vector = [NaN,NaN]

Does this PR introduce any user-facing change?
No
How was this patch tested?
added test

see https://issues.apache.org/jira/browse/SPARK-38584,
existing data validation should be unified, and this is the first PR to check LinearSVC.
friendly ping @srowen @huaxingao @WeichenXu123

just notice that UDF will skip null values, so directly using SQL functions is better since it can check NULL:
val df = sc.parallelize(Seq(("1", 2, 3.0), (null, 4, 5.0), ("0", 2, Double.PositiveInfinity))).toDF("a", "b", "c")

  def checkBinaryLabel = udf {
    label: Double =>
      require(label == 0 || label == 1,
        s"Labels MUST be in {0, 1}, but got $label")
      label
  }

scala> df.select(checkBinaryLabel(col("a").cast("double"))).show
+----------------------+
|UDF(cast(a as double))|
+----------------------+
|                   1.0|
|                  null|
|                   0.0|
+----------------------+

  def checkBinaryLabels(labelCol: String): Column = {
    val casted = col(labelCol).cast(DoubleType)
    when(casted.isNull, raise_error(lit("Labels MUST NOT be NULL")))
      .when(casted =!= 0 && casted =!= 1,
        raise_error(concat(lit("Labels MUST be in {0, 1}, but got "), casted)))
      .otherwise(casted)
  }

scala> df.select(checkBinaryLabels("a")).show
22/03/18 09:32:59 ERROR Executor: Exception in task 2.0 in stage 5.0 (TID 19)
java.lang.RuntimeException: Labels MUST NOT be NULL

@srowen
hmm, I plan to make other 4~5 PR for other impls:  other classification/regression/clustering/recommend/etc.

Is it possible to include those changes here? Those sound small and related. Rather than 4 or 5 of them

@srowen OK. I will have a try.

@srowen I found that it will touch too many files and some changes like to be impl-specific, what about limiting the scope of this PR to classification only?

Sure, just logical groupings. If we're going to have 4-5 1-file PRs, then combine them. If some are extensive changes in themselves, OK, separate PR

Looks OK. As far as you know, would this change behavior in any case? like something results in an error now that did not before?

yes. at least for LinearSVC, it will not throw an error before. I think we may need to notice this change in migration doc for 3.4.

What would happen, just silently get bad results?

What would happen, just silently get bad results?

yes, like the case in the description:
https://github.com/apache/spark/pull/35893#issue-1172201935

Merged to master, thanks all!


Calibrate delay axis

I am getting an error message while trying to calibrate_delay_axis.
It is looking for an ADC channel
KeyError: 'ADC'
which is not existing in our/FLASH files.
Even if I am trying to add as a parameter time0 and delay_range
sp.calibrate_delay_axis(time0=1463.6, delay_range=delay_range)
it is still looking for adc_range[0], adc_range[1], df[adc_column]

Yes, this function is specific to our imlementation converting the ADC channel into delay coordinates. I am not sure how the current version for flash has to look like (which channels have to be used). It would be good if we can come up with a combined function that does the correct thing depending on the config values.

Same applies probably for energy and momentum calibration functions for FLASH, as discussed previously. I am happy to help if I know why you guys need and want to use.

At FLASH, we are getting direct encoder position value in ps in our data stream (delayStage channel), so conversion is unnecessary. With hextof-processor we've used just time0 and delay range values for calibration.

I remember there was quite some elaborate code for e.g. filtering backwards/forwards scanning data, and also BAM correction etc. Is this all not required anymore? I suggest @zainsohail04 and/or @steinnymir, who know the FLASH code best, to have a look at this, and either suggest an independent function for FLASH, or integrate it with the mpes function (the funcionality could e.g. be made dependent on the availability of certain dataframe columns, or also simply the employed reader).


Fix pattern in Makefile to run Prettier correctly on Markdown files

📖 Description
When we changed file patterns #94 to use cd assets && npx prettier … command, we forgot to adjust the pattern for Markdown files.
We were checking the format for ./assets/**/*.md but most of our Markdown files are in ./, ./docs or .github.
📝 Notes
I tried to use the wider ../**/*.md pattern, but I couldn’t get Prettier to ignore files in ../deps (even by adding deps or ../deps to assets/.prettierignore 🤷♂) but I think this is good enough.

🦀 Requesting reviewers for this pull request:

@charlesdemers (contributor with 0 commits in the last 90 days and 2 commits overall)
@Boubalou (expert reviewer for the elixir stack)


use getOrCreate to get spark session to infer gpus

The getActiveSession() api returned None from multiprocessing.Pool process used on the driver in crossvalidation resulting in default value of 1 gpu being returned by _infer_num_workers.

build


make maximum split the grad like torch when equal

Cool! Can you add a test?

Can the eq.const_like(2).binary_op(BinaryOps.SUB, eq).binary_op(BinaryOps.DIV, eq.const_like(2)) be only written once?
The function isn't called if no shards are needed, so don't worry about that.

Cool, will merge if tests pass

In the failing test a comment reads "# TODO: this should be 4, but the sum output child stays around" and because doing the grad split requires and extra sum op with ".binary_op(BinaryOps.ADD, eq)" there is one more "sum output child" that gets left around. So I have added an 8 to the possible values for "len(GlobalCounters.cache)"


Docs inconsistent about opaque values from read-msg

According to the docs for read-msg, the function should return opaque values if the requested key corresponds to a list or object.
But in the REPL I am able to introspect the values:
(env-data { "admin-keyset": ["mockAdminKey"], "a-list": [1 2 3], "an-obj": {"k": "v"} })
(env-keys ["mockAdminKey"])
(define-keyset 'admins (read-keyset "admin-keyset"))

(module test 'admins
  (defun inspect-list ()
    (length (read-msg "a-list")))
  (defun inspect-obj ()
    (at 'k (read-msg "an-obj")))
)

(inspect-list)
(inspect-obj)

When this loads, it displays the following. Note the final two lines:
"Setting transaction data"
"Setting transaction keys"
"Keyset defined"
"Loaded module \"test\", hash \"a9e35e1b6f758da9547a8dc800f3116982688d5c6c7bce6c2996c69129cfe26e27fcd8eabf854532bd3ea85a2e67c6e3c42190a0753a4dff30b2717aa551c720\""
3
"v"

/cc @emilypi @joelburget @slpopejoy

Joel just pointed out that the docs here conflict with what the docs say under JSON support. So let's call this an inconsistency in the docs rather than a bug

I assume that the docs for read-msg are wrong in this case? @slpopejoy?


automation shall update pypi

our CI automation shall update pypi, too

base is a new configuration file where components to be updated in pypi are configured
tag rel/<current_componentversion> need to be created per repository
no automatic update if rel/<current_componentversion> is already existing.
This would mean that the latest component version is already available in pypi

Create/maintain Github workflow in repos to create Github Release and publish packages to PyPI

[ ] python-extensions-collection
[ ] python-genpackagedoc
[ ] python-jsonpreprocessor
[ ] python-pytestlog2db
[ ] python-microservice-base
[ ] python-microservice-cleware-switch
[ ] robotframework-extensions-collection
[ ] robotframework-qconnect-base
[ ] robotframework-testsuitesmanagement
[ ] robotframework-robotlog2db
[ ] robotframework-robotlog2rqm

New job/step in build pipeline (OSS or BIOS) to tag repos with rel/<current_componentversion>

[ ] Update git-tag tool to allow tag rel/x.x.x.x for Github repos
[ ] Update build pipeline to add new job to tag repos for publishing to PyPI

verified Upload to pypi:

[ ] python-extensions-collection
[ ] python-genpackagedoc
[ ] python-jsonpreprocessor
[ ] python-pytestlog2db
[ ] python-microservice-base
[ ] python-microservice-cleware-switch
[ ] robotframework-extensions-collection
[ ] robotframework-qconnect-base
[ ] robotframework-testsuitesmanagement
[ ] robotframework-robotlog2db
[ ] robotframework-robotlog2rqm

Update:
python-microservice-* repos is now removed from configuration for publishing to PyPI.

Done with merged PR https://github.com/test-fullautomation/RobotFramework_AIO/pull/89 and Publish <package> to PyPI workflow on corresponding repositories.

solved and already merged with #89


Wrong Profile Picture When Summoning Other Users' Profile

I am unsure whether this is a bug or whether it was intentional. Whenever you check a user's profile with $bal or $profile, it shows up their information. However, if you look to the top left, it shows your profile. Shouldn't it show the user's profile?

I believe it should. Thanks for pointing that out!


Fix gRPC test case

Signed-off-by: Isaac Hier<EMAIL_ADDRESS>
Which problem is this PR solving?

Resolves #1219.

Short description of the changes

Use port -1 and change expected error.

unit tests passed, merging


Test Issue created at Wed, 02 Jun 2021 12:42:01 GMT

Test issue body  Wed, 02 Jun 2021 12:42:01 GMT

Comment on issue at Wed, 02 Jun 2021 12:42:03 GMT


fix wrong scrollbar gap size

when body-scroll-lock is immediately called in parallel to an animation.
Fixes #188

Please make your change only to src/bodyScrollLock.js - the files in lib/ are built files.

Apologies - i just noticed you did patch src/


fix bug merge conflict

fixes: #8684

[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: geeknoid
If they are not already assigned, you can assign the PR to them by writing /assign @geeknoid in a comment when ready.
The full list of commands accepted by this bot can be found here.
The pull request process is described here

Needs approval from an approver in each of these files:

pkg/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

#8690 has fixed it


Global test number variable

Hi
Suppose I have 2 tests in file1.sh and 2 in file2.sh
bats file1.sh
will output 2 tests
bats file2.sh
will output 2 tests
and in each file, BATS_TEST_NUMBER is set as 1,2 for 1st and 2nd test respectively
BUT
bats file1.sh file2.sh
will output 4 tests in total
but BATS_TEST_NUMBER is still reset for individual files.
i.e
bats file1.sh file2.sh
ok 1
ok 2
ok 3   <- this will be from file2. BATS_TEST_NUMBER however here is '1'
ok 4
BATS_TEST_NUMBER is very useful to separate log files
but,
Is there a global test number apart from the per-file BATS_TEST_NUMBER which can be used in conjunction with this for large test case lists. This is because in systems like jenkins when lots of bats files are run, to quickly pick the log file from the global total test case number would be easy if there is a variable like BATS_GLOBAL_TEST_NUMBER  is set.

No, only the per-file test counter is available. The global test count that you see at the top, e.g. the 32 in 1..32, is computed (by bats-exec-suite) before any tests are ran (by bats-exec-test) and then discarded. The test number you see in each line of the TAP output is added by the TAP formatter (bats-format-tap-stream) that keeps count by counting the lines it receives after a test completes.
Please use markdown's formatting abilities, especially code blocks, to improve readability of your posts.


:fire_extinguisher: errorHandler middleware

Includes a broad range of error handling. However, we need to handle bad requests only, because other errors are being handled by the GPT-3 API.
Closes #2

Add error handler to openai.util.js for SERVICE_UNAVAILABLE when GPT-3 is unavailable.


Publish 08/04/2021, 10:30 AM

Docs Build status updates of commit 6baae27:
:white_check_mark: Validation status: passed

File
Status
Preview URL
Details

sharepoint/sharepoint-ps/sharepoint-online/Get-SPOSite.md
:white_check_mark:Succeeded
View (sharepoint-ps)

skype/skype-ps/skype/New-CsAutoAttendantMenuOption.md
:white_check_mark:Succeeded
View (skype-ps)

skype/skype-ps/skype/Set-CsTeamsMeetingPolicy.md
:white_check_mark:Succeeded
View (skype-ps)

For more details, please refer to the build report.
Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.
For any questions, please:Try searching the docs.microsoft.com contributor guidesPost your question in the Docs support channel


SSL Termination bug fixes

Schedule LB update on cert.remove only when lb is active

https://github.com/rancher/rancher/issues/1897

Propagate error message thrown by certificate validation code to the API

https://github.com/rancher/rancher/issues/1900

Bug in update load balancer

https://github.com/rancher/rancher/issues/1899

Cleanup certificates dir before applying new certs

https://github.com/rancher/rancher/issues/1904

Disallow cert removal when assigned to the LB service

https://github.com/rancher/rancher/issues/1906
Also fixed some NPE noticed in ServiceMetadataInfoFactory.java

flake8 fail

@cjellick  fixed, thank you

LGTM

LGTM


feat: update icon PR  details row

What type of PR is this? (check all applicable)

[x] 🍕 Feature
[ ] 🐛 Bug Fix
[ ] 📝 Documentation Update
[ ] 🎨 Style
[ ] 🧑💻 Code Refactor
[ ] 🔥 Performance Improvements
[ ] ✅ Test
[ ] 🤖 Build
[ ] 🔁 CI
[ ] 📦 Chore (Release)
[ ] ⏩ Revert

Description
This PR:

Updates the contributors profile page to include PRs related data
Updates global DbContribution types with extra values from the api

Notes:
The Open PRs percentage is still static and will be updated in a follow-up PR as it depend's on updates from the API
cc: @brandonroberts

Related Tickets & Documents
Fixes #696

Mobile & Desktop Screenshots/Recordings

Mobile

Added tests?

[ ] 👍 yes
[x] 🙅 no, because they aren't needed
[ ] 🙋 no, because I need help

Added to documentation?

[ ] 📜 README.md
[ ] 📓 docs.opensauced.pizza
[ ] 🍕 dev.to/opensauced
[ ] 📕 storybook
[x] 🙅 no documentation needed

[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?

@SunGoldTech, looking at the OP mock and I think we should remove icons and percents altogether. They are not adding value.
Please update the exact copy.

PRs Opened
PR Velocity
Contributed Repos

PRs reviewed are not possible right now.

PRs reviewed are not possible right now.

The data received from the API call for the useSingleContributor hook reflects a recent_pr_reviews field in the return object...
Question:

Is the recent_pr_reviews field not meant for the Prs Reviewed path?

cc: @bdougie

The recent_pr_reviews is assigned reviews, not completed reviews. We can decide whether that's useful to show or not

@OgDev-01 this one needs merge conflicts to be resolved also


Are there any plans to create streamdeck-atlassian with more application support?

Great plugin, thank you very much!
I am especially interested in Bitbucket. New plugin with ability to track status of pipelines or amount of merge requests would be very nice.

Sorry for the delay--I don't have any plans currently, unfortunately! I don't use Bitbucket in my organization so I don't have any repos or pipelines to test with. But I'm not against adding it to this plugin and would welcome any contributions!


(vietnamese) major translating

Apologize if this is too much, this took way longer than I imagine. The main thing about this PR is that it fully complete the entire achievement file for the Steam store page, and a mouthful of the strings in the main reactivedrop_vietnamese.txt file that is related to it, + the achievement strings for the in-game achievement tab and a few misc changes. Again, let me know if there is any problem, but keep in mind that I'm terrible at coding, and are still a novice on GitHub, so I might have some issues when solving problems
Also, should also mention this too so I don't have to create an issue, I saw that, despite already translated it, the workshop description remain untranslated when I switch to Vietnamese language. The workshop tags translation are already applied. Maybe have a look about it

Also, should also mention this too so I don't have to create an issue, I saw that, despite already translated it, the workshop description remain untranslated when I switch to Vietnamese language. The workshop tags translation are already applied. Maybe have a look about it

Good catch - should be fixed as soon as the cache expires.

All fixed. Well, aside from this as shown below, I look back and forth for error, and I still have no clue why this cause a fail check. Probably not a problem, I guess

I'll check what's going on locally after I merge.

Holy moly


Add possibility to remove "All Day" row in month and day view.

Sometimes business logic don't include All Day events.
It would be nice to have possibility to disable this functionality with a prop.

+1 for that! I am currently integrating the calendar into a project and there is no need for the All Day events.
Btw, great work!

I'd take a PR but you will run into the issue with multi-day (not all day) events, which right now are displayed in the all-day section, So I am not sure what you would do with those events in those cases.

It is easy to do by overriding styles:
.rbc-time-content {
    border-top: none;
    padding-top: 0px;
}
.rbc-time-header.rbc-header-overflowing > .rbc-row:nth-of-type(2) {
    display: none;
}

But yes, agree with that prop would save the time.

I'd appreciate this being implemented on the week view.
Multi-day events should be shown "uncollapsed", occupying all the time they encompass in the calendar.
We are using the calendar to let the user pick a time slot, but we need to blank out the existing events.
Thank you.

this worked for me
$('#calendar').find("tr .fc-unselectable:first").css( "display", "none" );


feat: Make sure to wait for DOM events

Summary
What:
Wait for:

Dom to be ready eg Dom.readyState
Allow users to wait a certain amount of time
- they can pass a callback to instead wait until a specific condition passed eg until button text equals something

Why:
We need to be sure that the Dom is loaded properly when the location is awaited, to test this, we could add a basic Dom content loaded handler that adds text to the Dom, assert we see the text
another thing I want to make sure we can cover is say the JS is what loads the content on the page eg API calls, React/Vue, where a basic view is displayed, and API calls are made once pagebis loaded to get a use rod users or whatever
There is also wait(‘$.active’) I used in Laravel, investigate this to see if it can help in waiting for these things
Acceptance Criteria
Below is a list of tasks that must be completed before this issue can be closed.

[ ] Write documentation
[ ] Write unit tests
[ ] Write integration tests
[ ] {add more checkboxes required by this issue as needed}

Example Pseudo Code (for implementation)
// Add example pseudo code for implementation

We do this already
Providing a wait function doesn't provide enough use, a user can use std/async/delay to wait
user can write their own logic if they want to wait
waiting for requests following click already covered by click method
waitinf for dom to load ie react components fetch data can be covered by std/async/delay

Be good to do what puppeteer does, via networkidle0, but i just have no clue how to implement it


Prepare for 1.4.3 Release

Many thanks to contributing to nf-core/rnaseq!
To ensure that your build passes, please make sure your pull request is to the dev branch rather than to master. Thank you!
Please fill in the appropriate checklist below (delete whatever is not relevant). These are the most common things requested on pull requests (PRs).
PR checklist

[x] PR is to dev rather than master
[ ] This comment contains a description of changes (with reason)
[x] If you've fixed a bug or added code that should be tested, add tests!
[ ] If necessary, also make a PR on the nf-core/rnaseq branch on the nf-core/test-datasets repo
[ ] Ensure the test suite passes (nextflow run . -profile test,docker).
[x] Make sure your code lints (nf-core lint .).
[ ] Documentation in docs is updated
[x] CHANGELOG.md is updated
[x] README.md is updated

Learn more about contributing: https://github.com/nf-core/rnaseq/tree/master/.github/CONTRIBUTING.md

Missing right now:

version bump
help / docs into JSON schema, though schema is verified and updated + grouped accordingly


[SPARK-5675][SQL] XyzType companion object should subclass XyzType

Otherwise, the following will always return false in Java.
dataType instanceof StringType

Test build #27070 has started for   PR 4463 at commit 976e11e.

This patch merges cleanly.

Test build #27072 has started for   PR 4463 at commit 04d5d8d.

This patch merges cleanly.

Test build #27070 has finished for   PR 4463 at commit 976e11e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27070/
Test PASSed.

Test build #27072 has finished for   PR 4463 at commit 04d5d8d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27072/
Test PASSed.


Decouple parseable-output from the driver and sink it into the frontend

Factor out the Driver-level dependencies on the parseable output generator and sink it down into the Frontend library. This is a step toward teaching the frontend to also emit parseable output, which is useful in the integrated Swift driver.

@swift-ci please smoke test

It's late & I'm tired. Is there an overall "why" comment somewhere, that explains that the purpose of having the frontend emit this format is so that Xcode (or xcbuild) can process the output for presentation in the IDE?

It's late & I'm tired, so maybe it's there already. Is there an overall "why" comment somewhere, that explains that the purpose of having the frontend emit this format is so that Xcode (or xcbuild) can process the output for presentation in the IDE?

I'll add a short comment to the header documentation.

@swift-ci please smoke test

@swift-ci please smoke test

@swift-ci smoke test

@swift-ci please smoke test

@swift-ci smoke test

@swift-ci please smoke test

@swift-ci clean smoke test Linux

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)


add interface to add new API hooks

EmuHelper class should expose a function that allows the user to add hooks for functions encountered during emulation. This is currently possible, but it requires knowledge of internals and is not documented.

https://github.com/fireeye/flare-emu/commit/3807dde962fedc9f58427c953e290a5feef24080