lint_yaml script can retired inactive + enahancements

@jamesturk / @csnardi
This PR updates lint_yaml.py to add an option to retire inactive person records (rather than report the error), and also a couple other enhancements, like validating person names for consistency and being able to pass the data root as an arg.
I ultimately needed these changes to make the airtable import/export workable, but I broken them out into a separate PR here because I think they're more complete and easier to review in isolation.
Let me know what you think.

feat: adds script to run tests with no db/django

feat: lint_yaml has option to retire inactive and can accept data_root as arg

feat: lint_yaml raises error for inconsistent person name

improvement: adds a pyenv .python-version file for latest 3.8

Thanks for this, a lot here, going to add some inline thoughts, but also a couple of questions.

is there a reason to have lint_yaml modify files, that seems a bit dangerous/unintuitive when retire.py could be modified to do the bulk retirement as needed?
can you explain data_root?
what is the logic with linting on inconsistent file names, the file name isn't an important part of the linting process by design, but curious to hear your thinking here

Thanks for this, a lot here, going to add some inline thoughts, but also a couple of questions.

is there a reason to have lint_yaml modify files, that seems a bit dangerous/unintuitive when retire.py could be modified to do the bulk retirement as needed?
can you explain data_root?
what is the logic with linting on inconsistent file names, the file name isn't an important part of the linting process by design, but curious to hear your thinking here

Thanks for this, a lot here, going to add some inline thoughts, but also a couple of questions.

is there a reason to have lint_yaml modify files, that seems a bit dangerous/unintuitive when retire.py could be modified to do the bulk retirement as needed?
can you explain data_root?
what is the logic with linting on inconsistent file names, the file name isn't an important part of the linting process by design, but curious to hear your thinking here

About the retire flag in lint_yaml, the advantage of putting it there is that the code that identifies (non-retired) person records with no active role already lives there, so it becomes a minor addition as opposed to a big refactoring and/or significant violation of DRY principals to move this logic to retire.py
I hear the concerns on this one, but I think these three points are mitigating:

you really can't enable this by accident, you  have to add a --retire flag to your lint_yaml call
it's familiar and common practice to add --fix flags to lint tools, e.g. eslint --fix
is there any common context where these scripts are run where changes can't just be reverted (i.e. aren't we generally running these scripts on git clones?)

Thanks for this, a lot here, going to add some inline thoughts, but also a couple of questions.

is there a reason to have lint_yaml modify files, that seems a bit dangerous/unintuitive when retire.py could be modified to do the bulk retirement as needed?
can you explain data_root?
what is the logic with linting on inconsistent file names, the file name isn't an important part of the linting process by design, but curious to hear your thinking here

About the retire flag in lint_yaml, the advantage of putting it there is that the code that identifies (non-retired) person records with no active role already lives there, so it becomes a minor addition as opposed to a big refactoring and/or significant violation of DRY principals to move this logic to retire.py
I hear the concerns on this one, but I think these three points are mitigating:

you really can't enable this by accident, you  have to add a --retire flag to your lint_yaml call
it's familiar and common practice to add --fix flags to lint tools, e.g. eslint --fix
is there any common context where these scripts are run where changes can't just be reverted (i.e. aren't we generally running these scripts on git clones?)

Thanks for this, a lot here, going to add some inline thoughts, but also a couple of questions.

is there a reason to have lint_yaml modify files, that seems a bit dangerous/unintuitive when retire.py could be modified to do the bulk retirement as needed?
can you explain data_root?
what is the logic with linting on inconsistent file names, the file name isn't an important part of the linting process by design, but curious to hear your thinking here

About the data_root: it's useful for the case where you're working on the scripts in some PR/branch, and you want to run the scripts to edit data in another branch/clone.
So when I was working on the mayor updates, I had my working code changes in this branch, and I would run the changes on another local clone of a branch called like mayor_updates. It was particularly convenient to be able to safely run like git reset --hard main on that mayor_update branch and/or I had to repeatedly merge/rebase that branch back onto updated main

Thanks for this, a lot here, going to add some inline thoughts, but also a couple of questions.

is there a reason to have lint_yaml modify files, that seems a bit dangerous/unintuitive when retire.py could be modified to do the bulk retirement as needed?
can you explain data_root?
what is the logic with linting on inconsistent file names, the file name isn't an important part of the linting process by design, but curious to hear your thinking here

About the data_root: it's useful for the case where you're working on the scripts in some PR/branch, and you want to run the scripts to edit data in another branch/clone.
So when I was working on the mayor updates, I had my working code changes in this branch, and I would run the changes on another local clone of a branch called like mayor_updates. It was particularly convenient to be able to safely run like git reset --hard main on that mayor_update branch and/or I had to repeatedly merge/rebase that branch back onto updated main

thanks for explaining the reasoning.  I'm on board with adding the name lint check if we can make the error message a bit more clear.
As for lint_yaml, judging by the big refactor here, the complexity of adding it seems like a lot more than refactoring it into retire.py.  I hear the DRY concerns, but a utility to check if people are retired would look like
def is_inactive(person, date=None):
    active = [role for role in person.get(roles_key, []) if role_is_active(role, date=date)]
    return len(active) == 0

I'm afraid I'm not getting the purpose of data_dir yet either, I have multiple copies checked out all the time too but couldn't you run the scripts from within the appropriate directory instead of relying on remembering to pass the flag?

thanks for explaining the reasoning.  I'm on board with adding the name lint check if we can make the error message a bit more clear.
As for lint_yaml, judging by the big refactor here, the complexity of adding it seems like a lot more than refactoring it into retire.py.  I hear the DRY concerns, but a utility to check if people are retired would look like
def is_inactive(person, date=None):
    active = [role for role in person.get(roles_key, []) if role_is_active(role, date=date)]
    return len(active) == 0

I'm afraid I'm not getting the purpose of data_dir yet either, I have multiple copies checked out all the time too but couldn't you run the scripts from within the appropriate directory instead of relying on remembering to pass the flag?

thanks for explaining the reasoning. I'm on board with adding the name lint check if we can make the error message a bit more clear.
As for lint_yaml, judging by the big refactor here, the complexity of adding it seems like a lot more than refactoring it into retire.py. I hear the DRY concerns, but a utility to check if people are retired would look like
def is_inactive(person, date=None):
    active = [role for role in person.get(roles_key, []) if role_is_active(role, date=date)]
    return len(active) == 0

I'm afraid I'm not getting the purpose of data_dir yet either, I have multiple copies checked out all the time too but couldn't you run the scripts from within the appropriate directory instead of relying on remembering to pass the flag?

Addressing the --data question:
Want to make sure we have a common understanding because sometimes when I read/review PR's I misinterpret what a change does.
So the number one thing is that the --data option is fully backwards compatible. if you don't pass it, then the scripts just function as they used to (assuming the data directory is ./data from the root of the openstates-people repo.
So unless I'm missing something, there are no additional burdens (e.g. new required arg to pass) and no downsides I can think of to having the --data option. And on the flip side, we get a lot in return for having this option. Advantages include these:
Enables end-to-end testing
All the tests added with this PR (and more in the airtable PR) are end-to-end minus the cli interface. You need to be able to change the data root to the custom example data root used by each test to be able to do this kind of end-to-end testing.
Makes working on scripts simultaneous with a data-update branch more practical
I know I mentioned this before, but didn't understand your reply about why it isn't better to have one branch for working on script changes and then do the actual data updates (run the scripts) on a fully separate branch and local clone. If I hadn't been able to do this, then all of my script changes would have been mixed in together with the mayor-updates PR I submitted a few weeks ago.
Re can you achieve the same by just running the scripts from target directory:
I don't think that would work. For one thing, the default way that data root is determined is by relative path from utils.py. And beyond that, PWD doesn't help with having a flexible data-root that can change to provide example data for each end-to-end test

thanks for explaining the reasoning. I'm on board with adding the name lint check if we can make the error message a bit more clear.
As for lint_yaml, judging by the big refactor here, the complexity of adding it seems like a lot more than refactoring it into retire.py. I hear the DRY concerns, but a utility to check if people are retired would look like
def is_inactive(person, date=None):
    active = [role for role in person.get(roles_key, []) if role_is_active(role, date=date)]
    return len(active) == 0

I'm afraid I'm not getting the purpose of data_dir yet either, I have multiple copies checked out all the time too but couldn't you run the scripts from within the appropriate directory instead of relying on remembering to pass the flag?

Addressing the --data question:
Want to make sure we have a common understanding because sometimes when I read/review PR's I misinterpret what a change does.
So the number one thing is that the --data option is fully backwards compatible. if you don't pass it, then the scripts just function as they used to (assuming the data directory is ./data from the root of the openstates-people repo.
So unless I'm missing something, there are no additional burdens (e.g. new required arg to pass) and no downsides I can think of to having the --data option. And on the flip side, we get a lot in return for having this option. Advantages include these:
Enables end-to-end testing
All the tests added with this PR (and more in the airtable PR) are end-to-end minus the cli interface. You need to be able to change the data root to the custom example data root used by each test to be able to do this kind of end-to-end testing.
Makes working on scripts simultaneous with a data-update branch more practical
I know I mentioned this before, but didn't understand your reply about why it isn't better to have one branch for working on script changes and then do the actual data updates (run the scripts) on a fully separate branch and local clone. If I hadn't been able to do this, then all of my script changes would have been mixed in together with the mayor-updates PR I submitted a few weeks ago.
Re can you achieve the same by just running the scripts from target directory:
I don't think that would work. For one thing, the default way that data root is determined is by relative path from utils.py. And beyond that, PWD doesn't help with having a flexible data-root that can change to provide example data for each end-to-end test

thanks for explaining the reasoning. I'm on board with adding the name lint check if we can make the error message a bit more clear.
As for lint_yaml, judging by the big refactor here, the complexity of adding it seems like a lot more than refactoring it into retire.py. I hear the DRY concerns, but a utility to check if people are retired would look like
def is_inactive(person, date=None):
    active = [role for role in person.get(roles_key, []) if role_is_active(role, date=date)]
    return len(active) == 0

I'm afraid I'm not getting the purpose of data_dir yet either, I have multiple copies checked out all the time too but couldn't you run the scripts from within the appropriate directory instead of relying on remembering to pass the flag?

RE: how much duplicate code required to migrate retire-inactive option from lint_yaml.py to its own isolated script:
I think it's a lot more than the one function you mention; you would need a new click command with args and then all the logic that (selectively, based on cli options) decides how to walk the data dir. So it would be a large portion of all of this:
def process_dir(
    abbr: str,
    data_root='',
    date=None,
    municipal = True,
    verbose = 0,
    retire_inactive = False,
    result_collector: Optional[ValidationResult] = None
) -> ValidationResult:  # pragma: no cover
    """
    Run linting on one directory at the level of an abbr (typically a state)

    Args:
        abbr (str): the state (or other region)
        data_root (str)[Optional]: root of people data, if left empty then `./data` folder in local clone
        date: (str)[Optional]: force a specific validation date
        municipal (bool)[Optional]: True to include municipalities in linting (default True)
        verbose (int)[Optional]: count of verbosity level (0 = None, 1 = on, 2 = more, etc)
        retire_inactive (bool)[Optional]: if True, try to fix "no active role" errors by retiring persons with no active role
        result_collection: (ValidationResult)[Optional] validation errors and warnings collected here. Will create and return an instance if none passed.

    Returns:
        (ValidationResult) has errors and warnings
    """
    data_root = data_root or get_data_root()
    legislative_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "legislature", "*.yml"))
    executive_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "executive", "*.yml"))
    municipality_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "municipalities", "*.yml"))
    retired_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "retired", "*.yml"))

    settings_file = os.path.join(os.path.dirname(__file__), "../settings.yml")
    with open(settings_file) as f:
        settings = load_yaml(f)
    try:
        validator = Validator(abbr, settings, data_root=data_root)
    except BadVacancy:
        sys.exit(-1)

    all_filenames = [
        (PersonType.LEGISLATIVE, legislative_filenames),
        (PersonType.RETIRED, retired_filenames),
        (PersonType.EXECUTIVE, executive_filenames),
    ]

    if municipal:
        all_filenames.append((PersonType.MUNICIPAL, municipality_filenames))

    to_retire = []
    ERROR_NO_ACTIVE_ROLES = "no active roles"
    for person_type, filenames in all_filenames:
        for filename in filenames:
            print_filename = os.path.basename(filename)
            with open(filename) as f:
                person = load_yaml(f)
                validator.validate_person(person, print_filename, person_type, date)
                if retire_inactive and ERROR_NO_ACTIVE_ROLES in validator.errors[print_filename]:
                    to_retire.append((person, filename, print_filename))
    if to_retire:
        # end_date is required for retire,
        # but in these cases will be unused
        # because we're retiring persons with no active role
        end_date = datetime.date.today().isoformat()
        for person_data, person_file, person_error_key in to_retire:
            retire_person_data(person_data, end_date, ERROR_NO_ACTIVE_ROLES)
            retire_person_file(person_file)
            validator.errors[person_error_key] = [
                err
                for err in validator.errors[person_error_key]
                if err != ERROR_NO_ACTIVE_ROLES
            ]
    return validator.print_validation_report(verbose, result_collector=result_collector)

@click.command()
@click.argument("abbreviations", nargs=-1)
@click.option(
    "--data",
    default=lambda: os.path.abspath(get_data_root())
)
@click.option(
    "--date", type=str, default=None, help="Lint roles using a certain date instead of today.",
)
@click.option(
    "--municipal/--no-municipal", default=True, help="Enable/disable linting of municipal data."
)
@click.option(
    "--retire/--no-retire", default=False, help="On encounter persons with 'no active roles' errors, try to retire those persons rather than fail"
)
@click.option("-v", "--verbose", count=True)
def lint(data, abbreviations, date, retire, municipal, verbose):
    """
        Lint YAML files.

        <ABBR> can be provided to restrict linting to single state's files.
    """
    error_count = 0

    if not abbreviations:
        abbreviations = get_all_abbreviations(data_root=data)

    result_collector = ValidationResult()
    for abbr in abbreviations:
        click.secho("==== {} ====".format(abbr), bold=True)
        process_dir(
            abbr,
            data_root=data,
            date=date,
            municipal=municipal,
            verbose=verbose,
            retire_inactive=retire,
            result_collector=result_collector
        )

...on this one as with the --data directory, I'm still not understanding the concrete downside to integrating this fix lint problem type feature into the lint script itself. It's common/expected for modern lint tools to offer the option to fix lint problems where the fix is automatable (e.g. eslint --fix)

thanks for explaining the reasoning. I'm on board with adding the name lint check if we can make the error message a bit more clear.
As for lint_yaml, judging by the big refactor here, the complexity of adding it seems like a lot more than refactoring it into retire.py. I hear the DRY concerns, but a utility to check if people are retired would look like
def is_inactive(person, date=None):
    active = [role for role in person.get(roles_key, []) if role_is_active(role, date=date)]
    return len(active) == 0

I'm afraid I'm not getting the purpose of data_dir yet either, I have multiple copies checked out all the time too but couldn't you run the scripts from within the appropriate directory instead of relying on remembering to pass the flag?

RE: how much duplicate code required to migrate retire-inactive option from lint_yaml.py to its own isolated script:
I think it's a lot more than the one function you mention; you would need a new click command with args and then all the logic that (selectively, based on cli options) decides how to walk the data dir. So it would be a large portion of all of this:
def process_dir(
    abbr: str,
    data_root='',
    date=None,
    municipal = True,
    verbose = 0,
    retire_inactive = False,
    result_collector: Optional[ValidationResult] = None
) -> ValidationResult:  # pragma: no cover
    """
    Run linting on one directory at the level of an abbr (typically a state)

    Args:
        abbr (str): the state (or other region)
        data_root (str)[Optional]: root of people data, if left empty then `./data` folder in local clone
        date: (str)[Optional]: force a specific validation date
        municipal (bool)[Optional]: True to include municipalities in linting (default True)
        verbose (int)[Optional]: count of verbosity level (0 = None, 1 = on, 2 = more, etc)
        retire_inactive (bool)[Optional]: if True, try to fix "no active role" errors by retiring persons with no active role
        result_collection: (ValidationResult)[Optional] validation errors and warnings collected here. Will create and return an instance if none passed.

    Returns:
        (ValidationResult) has errors and warnings
    """
    data_root = data_root or get_data_root()
    legislative_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "legislature", "*.yml"))
    executive_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "executive", "*.yml"))
    municipality_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "municipalities", "*.yml"))
    retired_filenames = glob.glob(os.path.join(get_data_dir(abbr, data_root=data_root), "retired", "*.yml"))

    settings_file = os.path.join(os.path.dirname(__file__), "../settings.yml")
    with open(settings_file) as f:
        settings = load_yaml(f)
    try:
        validator = Validator(abbr, settings, data_root=data_root)
    except BadVacancy:
        sys.exit(-1)

    all_filenames = [
        (PersonType.LEGISLATIVE, legislative_filenames),
        (PersonType.RETIRED, retired_filenames),
        (PersonType.EXECUTIVE, executive_filenames),
    ]

    if municipal:
        all_filenames.append((PersonType.MUNICIPAL, municipality_filenames))

    to_retire = []
    ERROR_NO_ACTIVE_ROLES = "no active roles"
    for person_type, filenames in all_filenames:
        for filename in filenames:
            print_filename = os.path.basename(filename)
            with open(filename) as f:
                person = load_yaml(f)
                validator.validate_person(person, print_filename, person_type, date)
                if retire_inactive and ERROR_NO_ACTIVE_ROLES in validator.errors[print_filename]:
                    to_retire.append((person, filename, print_filename))
    if to_retire:
        # end_date is required for retire,
        # but in these cases will be unused
        # because we're retiring persons with no active role
        end_date = datetime.date.today().isoformat()
        for person_data, person_file, person_error_key in to_retire:
            retire_person_data(person_data, end_date, ERROR_NO_ACTIVE_ROLES)
            retire_person_file(person_file)
            validator.errors[person_error_key] = [
                err
                for err in validator.errors[person_error_key]
                if err != ERROR_NO_ACTIVE_ROLES
            ]
    return validator.print_validation_report(verbose, result_collector=result_collector)

@click.command()
@click.argument("abbreviations", nargs=-1)
@click.option(
    "--data",
    default=lambda: os.path.abspath(get_data_root())
)
@click.option(
    "--date", type=str, default=None, help="Lint roles using a certain date instead of today.",
)
@click.option(
    "--municipal/--no-municipal", default=True, help="Enable/disable linting of municipal data."
)
@click.option(
    "--retire/--no-retire", default=False, help="On encounter persons with 'no active roles' errors, try to retire those persons rather than fail"
)
@click.option("-v", "--verbose", count=True)
def lint(data, abbreviations, date, retire, municipal, verbose):
    """
        Lint YAML files.

        <ABBR> can be provided to restrict linting to single state's files.
    """
    error_count = 0

    if not abbreviations:
        abbreviations = get_all_abbreviations(data_root=data)

    result_collector = ValidationResult()
    for abbr in abbreviations:
        click.secho("==== {} ====".format(abbr), bold=True)
        process_dir(
            abbr,
            data_root=data,
            date=date,
            municipal=municipal,
            verbose=verbose,
            retire_inactive=retire,
            result_collector=result_collector
        )

...on this one as with the --data directory, I'm still not understanding the concrete downside to integrating this fix lint problem type feature into the lint script itself. It's common/expected for modern lint tools to offer the option to fix lint problems where the fix is automatable (e.g. eslint --fix)

Thanks for the back & forth on this.  I can see how the data_dir thing could be useful for a feature as big as the airtable stuff... so even though it seems like a branch/commit of just data fixes would do this just as well 90% of the time, I can go along with the data_dir fix.
I'm still not sold on the need to increase the complexity of the lint_yaml script (which grew by 33%) being worth it to avoid having to type a few filenames on the command line or add a bit (yes a bit, the huge chunk of process dir you included is 75% unnecessary for retirement) to lint_yaml if we want it automated.  I've pushed a branch demonstrating what it'd look like in retire.py, it works fine but I'm not 100% sure of the UI yet.

Thanks for the back & forth on this.  I can see how the data_dir thing could be useful for a feature as big as the airtable stuff... so even though it seems like a branch/commit of just data fixes would do this just as well 90% of the time, I can go along with the data_dir fix.
I'm still not sold on the need to increase the complexity of the lint_yaml script (which grew by 33%) being worth it to avoid having to type a few filenames on the command line or add a bit (yes a bit, the huge chunk of process dir you included is 75% unnecessary for retirement) to lint_yaml if we want it automated.  I've pushed a branch demonstrating what it'd look like in retire.py, it works fine but I'm not 100% sure of the UI yet.


Modularized Bert2LSTM

Solves #57

I am not very familiar with tensorboard and hence did not include it here. I would request someone else to possibly take this up or maybe I can do it later.

I think this is simple enough to learn and it is going to be an essential thing in projects that you may work other than this.

That seems like a nice approach. Are you suggesting that we have a class for the model which handles its training/distillation, etc.?

Yes so basically all the functions related to, lets say bert go into one class.
I think after these changes we should be good with this implementation.

Yea seems a bit better but the size of the file is really huge. We will need to figure a way out to reduce that.

We would then need to write the training, distillation functions separately

We would then need to write the training, distillation functions separately

Yea I think we will have to figure out some way to do this efficiently.

We would then need to write the training, distillation functions separately

Yea I think we will have to figure out some way to do this efficiently.

I think we have the following options:

Create handler classes for LSTM and BERT
Create handler functions for LSTM and BERT
Derive a class from the base class for distilling to LSTM and then derive Bert2LSTM from that
Let me know your thoughts on this

Maybe we can sort this later when adding new implementations using these two models. That would give us more perspective on how to structure them

I think we have the following options:

Create handler classes for LSTM and BERT
Create handler functions for LSTM and BERT
Derive a class from the base class for distilling to LSTM and then derive Bert2LSTM from that
Let me know your thoughts on this

I really like this approach will be good for a start.

Maybe we can sort this later when adding new implementations using these two models. That would give us more perspective on how to structure them

Yea lets just get this implementation to work as of now, we will try to clean it up in another pr

Did you pull? I made changes to README.rst. I'll merge it after you pull the master branch.


TypeError: Cannot read property '1' of null

Currently the RegExp is not safe and at least a better error message should be visible in this case.
The documentation seems to be outdated as well as the output file is actually PID.clinic-flame.
Clinic Flame - v2.1.0 (0x v4.1.3)
...
You can then transfer the data and visualize it locally:

  clinic flame --visualize-only PID.flamegraph

clinic flame --visualize-only 13083.flamegraph

/home/ruben/.nvm/versions/node/v10.8.0/lib/node_modules/clinic/bin.js:256
    const name = filename.match(/clinic-([^-]+)/)[1]
                                                 ^

TypeError: Cannot read property '1' of null
    at viz (node/v10.8.0/lib/node_modules/clinic/bin.js:256:50)
    at runTool (node/v10.8.0/lib/node_modules/clinic/bin.js:228:5)
    at Command.func (node/v10.8.0/lib/node_modules/clinic/bin.js:176:34)
    at Command.call (node/v10.8.0/lib/node_modules/clinic/node_modules/commist/index.js:100:8)
    at Object.parse (node/v10.8.0/lib/node_modules/clinic/node_modules/commist/index.js:58:19)
    at Object.<anonymous> (node/v10.8.0/lib/node_modules/clinic/bin.js:182:4)

Fixed in #38


Feature/tri 1045 ess create testdata set

Added ESS test data set with failing BPN 'BPNL00000004FAIL'
Vehicle A has BPN chain BPNL00000003AYRE -> BPNL00000003AYRE -> BPNL00000003AYRE -> BPNL00000003B3NX -> BPNL00000003B0Q0
Vehicle B has BPN chain BPNL00000003AZQP -> BPNL00000004FAIL -> BPNL00000003AXS3
Adjusted script to create invalid EDC endpoint for BPN 'BPNL00000004FAIL'

BPNL00000003AZQP -> BPNL00000004FAIL -> BPNL00000003AXS3

BPNL00000003AZQP -> BPNL00000004FAIL -> BPNL00000003AXS3 (So in this case BPNL00000003AXS3  will never be reached )

New vehicle A structure


Implement KeySetPaginator

Q
A

Is bugfix?
❌

New feature?
✔

Breaks BC?
✔

Tests pass?
✔

Fixed issues

 Implement KeySetPaginator

Let's address https://github.com/yiisoft/yii-demo/pull/93#discussion_r418262635 separately, OK, @romkatsu?

Let's address #93 (comment) separately, OK, @romkatsu?

Ок


Add ninjaducks.in to showcase

 You should sign our Contributor License Agreement in order to get your pull request merged.

Cool website, @shivanshuag!
Merged as 1ce3bb2 (master) and 300ed74 (v0.14.docs), after I ran:
optipng -o7 -zm1-9 docs/static/img/ninjaducks-tn.png

to do a lossless optimization of your thumbnail:
Input file size = 69100 bytes
Output file size = 56680 bytes (12420 bytes = 17.97% decrease)

Cheers,
Anthony


amdvlk: Attempt to redefine function

I think it would useful to get open amdvlk working for comparisons against radv. It currently fails while handling SPIRV while running witness trace:
Error: Attempt to redefine function:
; Function Attrs: nounwind
declare spir_func
%spirv.SampledImage.float_1_0_0_0_1_0_0 addrspace(1)* @_Z12SampledImagePU3AS140__spirv_SampledImage_float_1_0_0_0_1_0_0i(%spirv.SampledImage.float_1_0_0_0_1_0_0 addrspace(1)*, i32) #0
 =>
%spirv.SampledImage.float_1_1_0_0_1_0_0 addrspace(1)* (%spirv.SampledImage.float_1_0_0_0_1_0_0 addrspace(1)*, i32)

Here is a snip from amdvlk icd/api/llpc/translator/SPIRVUtil.cpp , it looks like it doesn't like some type mismatch:
  FunctionType *FT = FunctionType::get(RetTy, ArgTypes, isVarArg);
  Function *F = M->getFunction(MangledName);
  if (!takeName && F && F->getFunctionType() != FT && Mangle != nullptr) {
    std::string S;
    raw_string_ostream SS(S);
    SS << "Error: Attempt to redefine function: " << *F << " => " <<
        *FT << '\n';
    report_fatal_error(SS.str(), false);
  }

This particular issue should be fixed when applying GPUOpen-Drivers/xgl#6.


ISSUE-8 Add Travis CI build

Add Travis CI build configuration file to enable build on PRs as well as branches.
close #8

Circle CI doesn't take this PR. Recreate may works?


Sample json data incorrect

In the following sample json file, https://github.com/eventbrite/britecharts/blob/master/test/json/lineDataOneSet.json
It appears that there's duplicate, or incorrect date keys in the data.
 {
                   "date": "2016-07-31T16:00:00-08:00",
                   "value": 0,
                   "date": "2016-07-31T00:00:00-07:00"
 ,

Expected Behavior
Current Behavior
Possible Solution

Steps to Reproduce

Look at code here https://github.com/eventbrite/britecharts/blob/master/test/json/lineDataOneSet.json

Screenshots (if appropriate)
Context

Your Environment

Britecharts version used:
Browser Name and version:
Operating System and version:
Link to your project or codepen:

Hi @flacoman91, thanks for pointing this out!
Yeah, they only change the hour, we should probably clean it!

Fixed on https://github.com/britecharts/britecharts/pull/831


fix: psbt bare utxo signing

Try out this version of the Hiro Wallet - download extension builds.

Solution to handle bare UTXO signing.
The app has to initialize the tx with disableScriptCheck: true, and the changes here would handle getting the address from script type 'ms' w/out errors.
Todo:

[x] Put up issue in btc-signer lib to handle all types there.

EDIT: Issue opened here: https://github.com/paulmillr/scure-btc-signer/issues/55

@kyranjamie what do you think about this response?
https://github.com/paulmillr/scure-btc-signer/issues/55

Where does ms come from in the first place @fbwoolf ?

Where does ms come from in the first place @fbwoolf ?

ms is the outscript type supplied by the app trying to do multisig. Apparently, though, they are doing it wrong, see this comment on issue: https://github.com/hirosystems/wallet/issues/3897#issuecomment-1658998229


feat(component): enhance chat input component

Description
This PR enhances the Chat Input component by simplifying the input structure and improving text handling.

Changes

Improved Text Handling:

Replaced MultilineInput with StrInput for more efficient text management.

Playground Integration:

Removed input handle to streamline functionality and enhance integration with the playground.

Image and File Support:

Maintained support for images and text files through FileInput using TEXT_FILE_TYPES + IMG_FILE_TYPES.

Styling and Session Management:

Retained essential styling and session management options to ensure consistent user experience.

Code Comparison
Before
inputs = [
    MultilineInput(
        name="input_value",
        display_name="Text",
        value="",
        info="Message to be passed as input.",
    ),
    # ...
]

After
inputs = [
    StrInput(
        name="input_value",
        display_name="Text",
        value="",
        info="Message to be passed as input.",
    ),
    # ...
]

Benefits

Simplified code for better readability and maintenance.
Enhanced compatibility with the playground environment.
Continued support for multimedia input types.
Improved overall user experience and functionality.

What are the advantages of using a StrInput over a MultilineInput for this case?

The main reason for using StrInput instead of MultilineInput is that both provide the same functionality, but StrInput does not include a handle. This makes the Chat Input component cleaner and more streamlined.
Additionally, as the Chat Input is a primary component, having a handle is unnecessary.
Removing the handle ensures a simpler and more intuitive design, focusing on the component's core functionality.


SubscriberClient.Create does not exist from the 1.0.0-beta20

I want update my package : Google.Cloud.PubSub from 1.0.0-beta19 to 1.0.0.
But after this upgrade  SubscriberClient.Create doesn't exist anymore. What I can use instead in my code ?
SubscriptionName subscriptionName = new SubscriptionName(projectId, subscriptionId);
SubscriberServiceApiClient subscriberClient = SubscriberServiceApiClient.Create(channel);
SubscriberClient subscriber = SubscriberClient.Create(subscriptionName, new[] { subscriberClient });

Do you have a link to a changelog?
Environment details

OS: Windows 10
.NET version: 2.1
Package name and version: Google.Cloud.PubSub 1.0.0

Thanks to everyone

Chris can provide more details, but it looks like you want to use the [SubscriberClient.ClientCreationSettings] type to pass into SubscriberClient.CreateAsync.

You can call the constructor directly:
SubscriberClient subscriber = new SubscriberClientImpl(subscriptionName, new[] { subscriberClient }, new Settings(), null);

Or don't create a SubscriberServiceApiClient yourself; instead call SubscriberClient.CreateAsync.CreateAsync(subscriptionName);

@xav-stargate: Have our comments resolved this? If not, please provide more information about what you're looking for.

Closing as I believe we've addressed this - @xav-stargate please add another comment if you need more information.

Yes @jskeet thank you =)
Sorry for the delay


Actually create the hook

When creating a repository, we will create a hook using credentials set up via arguments.

This also works now.


[Story] Add API functionality to delete user-resource

Description
As a TRE owner / researcher / user
I want to be able to delete a user resource associated to a workspace-service
So that all Azure resources of this user-resources are no longer deployed to Azure.
Acceptance criteria

[ ] TRE owner / researcher can delete user-resource that was been disabled --DELETE
/api/workspaces/{workspace_id}/workspace-services/{service_id}/user-resources
[ ] if attempting to delete enabled user-resource this should return an error stating user-resource must be disabled before being deleted.
[ ] When deleting a user-resource -
Cloud resources are removed
user-resources status changes to deleted
user-resource is not available through -- GET  /api/workspaces/{workspace_id}/workspace-services/{service_id}/user-resources

Removal of resources to be implemented separately in each bundle using the uninstall action


integrate isort into your extension

Your python extension has integrated many python tools, including Autopep8, Black, Yapf, Bandit, Flake8, Mypy, Pycodestyle, Pydocstyle, Pylama, Pylint and so on. For each integrated tool, we can configure it in your extension settings. In my opinion, there is nothing special about isort. You are not necessary to let isort as a separate extension, and we are necessary to install a separate extension for isort's little function.

We are actually either dropping or moving all of those other tools out of the Python extension and into their own extension (e.g. autopep8, black, flake8, and pylint already have their own extensions). So you're right that isort isn't special, but that means we moved it out like we are doing for all the other tools you listed instead of keeping it internal to the Python extension.


fix Fix update rows for unique table  didn't get updated properly

Proposed changes
Issue Number: close #xxx

Further comments
If this is a relatively large or complex change, kick off the discussion at<EMAIL_ADDRESS>by explaining why you chose the solution you did and what alternatives you considered, etc...

https://github.com/apache/doris/pull/26968/files

run buildall

run buildall


[MYSQL] az mysql flexible-server deploy/update: Add user confirmation and input validator

Related command

az mysql flexible-server deploy/update
Description

az mysql flexible-server deploy run: add input validator to fix security issue.
az mysql flexible-server update: add user confirmation for HA enabled.

This checklist is used to make sure that common guidelines for a pull request are followed.

[x] The PR title and description has followed the guideline in Submitting Pull Requests.

[ ] I adhere to the Command Guidelines.

[ ] I adhere to the Error Handling Guidelines.

MYSQL


Fix link to tcn-coalition (resolves #1847)

This PR resolves the issue https://github.com/corona-warn-app/cwa-website/issues/1847 "TCN Coalition website moved" by changing TCN links from
https://tcn-coalition.org/
to
https://www.lfph.io/tcn-coalition/.
Steps to verify
Local verification

Open http://localhost:8000/en/ and click on link to TCN
Open http://localhost:8000/de/ and click on link to TCN
npm run test with no error

@MikeMcC399 Thanks.


[AST] Remove ParameterTypeFlags from ParenType and TupleType

The last clients that relied on stashing parameter type flags on these types are now gone.

@swift-ci please test

@swift-ci please test source compatibility

@swift-ci please test

@swift-ci please test source compatibility

GitHub got the wrong timestamps for the force push, correct diff is https://github.com/apple/swift/compare/dab78c634d2adfc350f8f5085fce358040a4d2c8..69f59bbc6c8b2201b64b0100a8a62bffb79775e2

@swift-ci please test

@xedin Noticed that we didn't have logic to specifically reject inout arguments for tuple construction (previously we were relying on TupleTypeElt::getType producing InOutType to fail the match). Added logic to reject inout arguments up-front in 1bf954c61feaed20a7020f91739fc93e49a1c333, mind taking a look?

@hamishknight Looks great!

:ship:


Instructions missing/updates

I've followed the instructions on the main page and ran into a several issues:

The latest version of Google Sign-In SDK is 4.1.2 and it does not include GoogleSignInDependencies.framework.
When I do the "Add files to '{Your App}'" and pick {YourApp}/node_modules/react-native-google-sign-in/ios/RNGoogleSignIn, the folder shows up in blue and not yellow/orange. I think this is a significant difference. Could the instructions be updated to make sure the imported files are included in the project? (see bellow)
Running the app in iOS gives an error on "import GoogleSignIn from 'react-native-google-sign-in;" indicating that the code added above is not being compiled.

I hope the instructions can be updated to help with the above. I'm a very well versed Android programmer but this is my first serious venture into iOS and a more experienced iOS programmer could probably overcome the above issue. Help with it would probably benefit other users.

@FunMiles I have the same issue. Apparently you have to add New Group instead of Files, it will create a yellow folder and then you can add the files to it. This solves my runtime error.


feat(be): Allow multiple Ansible vaults

This adds the ability to specify multiple Ansible vaults during a Task run.
It changes the UI to allow specifying multiple Access Keys with an associated name field. If name isn't specified, the vault is assumed to be the default. Only one default vault is allowed. Multiple vaults will be passed to Ansible using --vault-id={name}@prompt. I've also added support to backup/restore as well as remote runners.
See https://docs.ansible.com/ansible/2.9/user_guide/vault.html#multiple-vault-passwords
Fixes https://github.com/semaphoreui/semaphore/issues/1002

Wow, really cool feature! Thanks a lot! :) I reviewing it.


TensorFlow UnboundLocalError: local variable 'batch_outputs' referenced before assignment

I am trying to run some python3 code on databricks GPU cluster for image understanding by CNN.
The env:
   TensorFlow: 2.2
   python 3.7.6
   keras: 2.3.0-tf
   Unbuntu: 4.4

The code:
import os
import numpy as np
from tensorflow.keras.preprocessing.image import ImageDataGenerator

base_dir = '/Users/Downloads/cats_and_dogs_small'

train_dir = os.path.join(base_dir, 'train')
validation_dir = os.path.join(base_dir, 'validation')
test_dir = os.path.join(base_dir, 'test')

datagen = ImageDataGenerator(rescale=1./255)
batch_size = 20

def extract_features(directory, sample_count):
    features = np.zeros(shape=(sample_count, 4, 4, 512))
    labels = np.zeros(shape=(sample_count))
    generator = datagen.flow_from_directory(
        directory,
        target_size=(150, 150),
        batch_size=batch_size,
        class_mode='binary')
    i = 0
    for inputs_batch, labels_batch in generator:
        features_batch = conv_base.predict(inputs_batch) # error !
        features[i * batch_size : (i + 1) * batch_size] = features_batch
        labels[i * batch_size : (i + 1) * batch_size] = labels_batch
        i += 1
        if i * batch_size >= sample_count:
            # Note that since generators yield data indefinitely in a loop,
            # we must `break` after every image has been seen once.
            break
    return features, labels

train_features, train_labels = extract_features(train_dir, 2000). # error here !
validation_features, validation_labels = extract_features(validation_dir, 1000)
test_features, test_labels = extract_features(test_dir, 1000)

the call stack:
Found 0 images belonging to 0 classes.
UnboundLocalError: local variable 'batch_outputs' referenced before assignment
---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)
<command-2528158> in <module>
     33     return features, labels
     34
---> 35 train_features, train_labels = extract_features(train_dir, 2000)
     36 validation_features, validation_labels = extract_features(validation_dir, 1000)
     37 test_features, test_labels = extract_features(test_dir, 1000)

<command-2528158> in extract_features(directory, sample_count)
     23     i = 0
     24     for inputs_batch, labels_batch in generator:
---> 25         features_batch = conv_base.predict(inputs_batch)
     26         features[i * batch_size : (i + 1) * batch_size] = features_batch
     27         labels[i * batch_size : (i + 1) * batch_size] = labels_batch

/databricks/python/lib/python3.7/site-packages/tensorflow/python/keras/engine/training.py in _method_wrapper(self, *args, **kwargs)
     86       raise ValueError('{} is not supported in multi-worker mode.'.format(
     87           method.__name__))
---> 88     return method(self, *args, **kwargs)
     89
     90   return tf_decorator.make_decorator(

/databricks/python/lib/python3.7/site-packages/tensorflow/python/keras/engine/training.py in predict(self, x, batch_size, verbose, steps, callbacks, max_queue_size, workers, use_multiprocessing)
   1283             callbacks.on_predict_batch_end(step, {'outputs': batch_outputs})
   1284       callbacks.on_predict_end()
-> 1285     all_outputs = nest.map_structure_up_to(batch_outputs, concat, outputs)
   1286     return tf_utils.to_numpy_or_python_type(all_outputs)
   1287

UnboundLocalError: local variable 'batch_outputs' referenced before assignment

@umusa
Please, provide sample data to reproduce the issue in our environment.Thanks!

Hi, the data is at www.kaggle.com/c/dogs-vs-cats/data, you may need to download the zip file that contain cat and dog images.
The py3 code that I am using is at https://github.com/fchollet/deep-learning-with-python-notebooks/blob/master/5.3-using-a-pretrained-convnet.ipynb
thanks

@umusa
I have tried in colab with TF version 2.3 and i am seeing the below error message.
(AttributeError: 'int' object has no attribute 'assign').
Please, find the gist here.Thanks!

@umusa did the find the solution ? I am facing the same error

I have the same problem when I passed an empty input set to the predict method. A better error message really would help to avoid confusion.

@177arc  that's correct. This error is related to passing  empty array to the predict method.


csv-values: Pipes in data are seen as delimiter between check output and performance data

This issue respects the following points:

[X] This is a bug, not a question or a setup/configuration issue.
[X] This issue is not already reported on Github (I've searched it).
[X] I use the latest release of the Monitoring Plugins (https://github.com/Linuxfabrik/monitoring-plugins/releases).
[X] I agree to follow Monitoring Plugins's Code of Conduct.

Which variant of the Monitoring Plugins do you use?

[ ] .rpm/.deb package from repo.linuxfabrik.ch
[ ] Compiled for Linux (.tar/.zip from download.linuxfabrik.ch)
[X] Compiled for Windows (from download.linuxfabrik.ch)
[ ] Source Code from GitHub

Bug description
While setting up a check to monitor long running LDAP queries on the domain controllers I discovered the following behavior:
All lines were cut after the first |
I tried to mitigate with replace( Filter, "|", "¦" ) in the --warning-query and now the results aren't cut any longer but the perfdata is still mangled ) ) ! 3305761 ! 24 ! INTERSECT_INDEX:34754:I;|'cnt_warn'
Steps to reproduce - Plugin call
'C:\ProgramData\icinga2\usr\lib64\nagios\plugins\csv-values.exe' '--chunksize' '1000' '--columns-query' 'LDAPServer TEXT,TimeGenerated TEXT,ClientIP TEXT,ClientPort INTEGER,StartingNode TEXT,Filter TEXT,SearchScope TEXT,AttributeSelection TEXT,ServerControls TEXT,VisitedEntries INTEGER,ReturnedEntries INTEGER,UsedIndexes TEXT,PagesReferenced TEXT,PagesReadFromDisk INTEGER,PagesPreReadFromDisk INTEGER,CleanPagesModified INTEGER,DirtyPagesModified INTEGER,SearchTimeMS INTEGER,AttributesPreventingOptimization TEXT' '--delimiter' ',' '--filename' 'D:\Downloads\ICTADC22WP01\1644-ictadc22wp01_20231201-0159_DirectoryService.csv' '--quotechar' '"' '--timeout' '3' '--warning' '0' '--warning-query' 'select TimeGenerated,ClientIP,SearchTimeMS,StartingNode,replace( Filter, "|", "¦" ),VisitedEntries,ReturnedEntries,UsedIndexes from data where SearchTimeMS >=20000 order by SearchTimeMS DESC'
Steps to reproduce - Data

get some CSV with |s in the data

generate eventlogs according to https://learn.microsoft.com/en-US/troubleshoot/windows-server/identity/event1644reader-analyze-ldap-query-performance
use the part in Event1644Reader.ps1 before the excel generation the CSV files
or use or craft any other CSV with | in the data

setup a csv-values based service

Environment
OS Name	Microsoft Windows Server 2022 Standard
Version	10.0.20348 Build 20348
Plugin Version
csv-values.exe: v2023051201 by Linuxfabrik GmbH, Zurich/Switzerland
Python version
No response
List of Python modules
No response
Additional Information
No response

Also happens for URLs in php-fpm-status messages:
Plugin-Ausgabe
Pool www (dynamic): 2x max children reached [WARNING], 1.6M connections, 35 processes (2 active, 33 idle), Up 1W 6D (since 2024-03-07 04:11:25)
PID     ! Reqs ! ReqDur      ! Request URI                                                                                ! POST !
Performancedaten
  | Bezeichnung | Wert | Max | Warnung | Kritisch
-- | -- | -- | -- | -- | --
  | service_display_name | - | - | - | -
  | ! - ! - service | - | - | - | -
  | ! - ! - hostgroup_alias | - | - | - | -
  | ! - ! - servicegroup_alias | - | - | - | -
  | max children reached | 2.00 | - | 1.00 | 100.00
  | idle processes | 30.00 | 35.00 | - | -
  | active processes | 5.00 | 35.00 | - | -
  | ! - ! -\| | 1,647,676.00 c | - | - | -
  | listen queue | 0.00 | 0.00 | - | -
  | listen queue len | 0.00 | - | - | -
  | slow requests | 0.00 | - | 1.00 | 100.00
  | start since | 13,24 d | - | - | -
  | queue usage | 0% | - | 80% | 90%

maybe add escaping / replacing in lib.base.oao?

Fixed in https://github.com/Linuxfabrik/lib/commit/c9072e3a537054e0c5b57899a17385f4bb860698


Add PWA support

Consider finishing off missing features for general PWA compatibility

As listed in the image, there are only a few things needed to finish the general implementation.
The check was generated using Lighthouse

This is the result of the Lighthouse check running on the PR on localhost (hence the HTTPS redirect "error").

Closed with #179


'loglevel' dependency missing on npm installs

Frodo CLI version
This will currently be 2.0.0 as in our Dockerfile we are running RUN npm i -g @rockcarver/frodo-cli which will be pulling the latest.
Describe the issue
On a Docker image where we have installed the CLI globally, trying to run an import gives the following:
`node:internal/modules/cjs/loader:1140
const err = new Error(message);
^
Error: Cannot find module 'loglevel'
Require stack:

<EMAIL_ADDRESS>at Module._resolveFilename (node:internal/modules/cjs/loader:1140:15)
at Module._load (node:internal/modules/cjs/loader:981:27)
at Module.require (node:internal/modules/cjs/loader:1231:19)
at require (node:internal/modules/helpers:177:18)
at Object. (/usr/local/lib/node_modules/@rockcarver/frodo-cli/node_modules/@rockcarver/frodo-lib/src/api/BaseApi.ts:9:28)
at Module._compile (node:internal/modules/cjs/loader:1364:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1422:10)
at Module.load (node:internal/modules/cjs/loader:1203:32)
at Module._load (node:internal/modules/cjs/loader:1019:12)
at ModuleWrap. (node:internal/modules/esm/translators:203:29) {
code: 'MODULE_NOT_FOUND',
requireStack: [<EMAIL_ADDRESS>]
}`

This is mitigated by also globally installing the loglevel dependency: RUN npm i -g  loglevel

@conorbarford-ping this is fixed in Frodo CLI 2.0.2. Please let me know if you are still seeing the issue.


Suppress max/min macros in windows.h

Using MS Visual Studio, when you #include <windows.h>, some preprocessor macros are defined by default for min() and max(). As a result, windows.h interferes with the C++ STL, for example, std::numeric_limits::min() and std::max() in the algorithm header.
See minwindef.h for the offending code. This header is included by windef.h, which is included by windows.h. Thankfully, these definitions of min and max are surrounded by an #ifndef NOMINMAX, so we have a ready solution to the problem.
Alternatives: You could just not ever use the Visual Studio compiler; the windows.h file that ships with my MSys2 mingw-gcc does not have this problem. You could use some preprocessor #defines and/or #undefs in your otherwise pristine ISO-compliant C++ code. I prefer to use good cmake defaults that prevent this kind of thing from ever showing up. You shouldn't ever have to think about weird stuff like this.
Potential for improvement: I think that it might be worth moving this kind of code to a separate file in the cmake folder, perhaps called cmake/Definitions.cmake, but I'm not sure that's necessary.
Background: I just ran into this conflict between windows.h and std::numeric_limits in another project. When I googled the two terms together, I was so mad that so many other people complaining about this problem that I had to do something. Please, let's have sane defaults that make it easier to use the C++ STL, and not clobber the std namespace with legacy preprocessor macros!

I thought I should add some clarification as to why I think this PR belongs in the project, and what exactly this intends to fix.
Consider the following code:
#include <limits>
#include <windows.h>

constexpr auto get_amt() { return std::numeric_limits<int>::max(); }

When I compile this file with mingw-gcc or with clang on windows, without linking, compilation is successful. If I compile it with cl.exe (the MS Visual Studio compiler), compilation fails and I get this error:
limits.cpp(4): warning C4003: not enough arguments for function-like macro invocation 'max'
limits.cpp(4): error C2589: '(': illegal token on right side of '::'
limits.cpp(4): error C2062: type 'unknown-type' unexpected
limits.cpp(4): error C2059: syntax error: ')'

If I remove the #include <windows.h> line, cl.exe compiles the file correctly. This is because windows.h defines min and max as macros, and the preprocessor has no choice but to substitute the word max for that macro before evaluating my code.
This is not due to a bug in cl.exe or in windows.h. This is due to a design choice made decades ago by Microsoft, before std::max existed, and Microsoft cannot reverse that choice without breaking people's code.
The cleanest fix to this problem, other than abandoning MSVC (not an option for many of us), is to add the definition NOMINMAX to your project. This definition prevents the min and max macros from being defined. My solution puts that definition in a CMakeLists.txt file, upstream of any code where it would be needed. It's so far out of the way that, unless you go looking for it, you'll never even know this was a problem: you can just get to work, using std::max and std::numeric_limits::max without any problem.
Maybe this fix doesn't belong in this project. Maybe the text NOMINMAX has some meaning in some other library I don't know about, and by including this definition I am breaking someone else's project, though I doubt it. If so, I will happily close this PR unmerged.
To my understanding, the purpose of this project is to make it easier to write clean C++ that follows best-practices as much as possible. Once in a while, some of us have to get into the weeds with some platform-specific Windows code; when that happens, the NOMINMAX definition will be necessary in order to use some features of the STL. If you're not familiar with all the pitfalls involved with windows.h, you won't know what's happening when you see the error message listed above. This PR will prevent people from dealing with that headache, and make it more natural to write good C++.

I thought I should add some clarification as to why I think this PR belongs in the project, and what exactly this intends to fix.
Consider the following code:
#include <limits>
#include <windows.h>

constexpr auto get_amt() { return std::numeric_limits<int>::max(); }

When I compile this file with mingw-gcc or with clang on windows, without linking, compilation is successful. If I compile it with cl.exe (the MS Visual Studio compiler), compilation fails and I get this error:
limits.cpp(4): warning C4003: not enough arguments for function-like macro invocation 'max'
limits.cpp(4): error C2589: '(': illegal token on right side of '::'
limits.cpp(4): error C2062: type 'unknown-type' unexpected
limits.cpp(4): error C2059: syntax error: ')'

If I remove the #include <windows.h> line, cl.exe compiles the file correctly. This is because windows.h defines min and max as macros, and the preprocessor has no choice but to substitute the word max for that macro before evaluating my code.
This is not due to a bug in cl.exe or in windows.h. This is due to a design choice made decades ago by Microsoft, before std::max existed, and Microsoft cannot reverse that choice without breaking people's code.
The cleanest fix to this problem, other than abandoning MSVC (not an option for many of us), is to add the definition NOMINMAX to your project. This definition prevents the min and max macros from being defined. My solution puts that definition in a CMakeLists.txt file, upstream of any code where it would be needed. It's so far out of the way that, unless you go looking for it, you'll never even know this was a problem: you can just get to work, using std::max and std::numeric_limits::max without any problem.
Maybe this fix doesn't belong in this project. Maybe the text NOMINMAX has some meaning in some other library I don't know about, and by including this definition I am breaking someone else's project, though I doubt it. If so, I will happily close this PR unmerged.
To my understanding, the purpose of this project is to make it easier to write clean C++ that follows best-practices as much as possible. Once in a while, some of us have to get into the weeds with some platform-specific Windows code; when that happens, the NOMINMAX definition will be necessary in order to use some features of the STL. If you're not familiar with all the pitfalls involved with windows.h, you won't know what's happening when you see the error message listed above. This PR will prevent people from dealing with that headache, and make it more natural to write good C++.


Added a warmer option to prepopulate the map before usage

Coverage remained the same at 100.0% when pulling 9c956b46e519ce6e0a3d8aef1e41518edc104391 on rtward:master into 2bd22a21a857a90e24ac56ec2008b08abf88c440 on bchociej:master.


ccextractor: 0.92 -> 0.93

Automatic update generated by nixpkgs-update tools. This update was made based on information from https://github.com/CCExtractor/ccextractor/releases.
meta.description for ccextractor is: "Tool that produces subtitles from closed caption data in videos"
meta.homepage for ccextractor is: "https://www.ccextractor.org"
meta.changelog for ccextractor is: ""
Updates performed

Version update

To inspect upstream changes

Release on GitHub

Compare changes on GitHub

Impact

Checks done (click to expand)

built on NixOS

The tests defined in passthru.tests, if any, passed

/nix/store/spajxyizs6nqyx19rzpzlvkrmja3jysx-ccextractor-0.93/bin/ccextractor passed the binary check.

/nix/store/spajxyizs6nqyx19rzpzlvkrmja3jysx-ccextractor-0.93/bin/.ccextractor-wrapped passed the binary check.

2 of 2 passed binary check by having a zero exit code.

2 of 2 passed binary check by having the new version present in output.

found 0.93 with grep in /nix/store/spajxyizs6nqyx19rzpzlvkrmja3jysx-ccextractor-0.93

found 0.93 in filename of file in /nix/store/spajxyizs6nqyx19rzpzlvkrmja3jysx-ccextractor-0.93

directory tree listing: https://gist.github.com/ce6883f3373578f6e52cd7a66299434c

du listing: https://gist.github.com/f52b0fb3cc2b7789aeaf18dfdf668473

Rebuild report (if merged into master) (click to expand)

4 total rebuild path(s)

1 package rebuild(s)

1 x86_64-linux rebuild(s)
1 i686-linux rebuild(s)
1 x86_64-darwin rebuild(s)
1 aarch64-linux rebuild(s)

First fifty rebuilds by attrpath
ccextractor

Instructions to test this update (click to expand)

Either download from Cachix:
nix-store -r /nix/store/spajxyizs6nqyx19rzpzlvkrmja3jysx-ccextractor-0.93 \
  --option binary-caches 'https://cache.nixos.org/ https://nix-community.cachix.org/' \
  --option trusted-public-keys '
  nix-community.cachix.org-1:mB9FSh9qf2dCimDSUo8Zy7bkq5CX+/rkCWyvRCYg3Fs=
  cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=
  '

(The Cachix cache is only trusted for this store-path realization.)
For the Cachix download to work, your user must be in the trusted-users list or you can use sudo since root is effectively trusted.
Or, build yourself:
nix-build -A ccextractor https://github.com/r-ryantm/nixpkgs/archive/7ed3f15b7d8212f4394f282565c42802abb45d19.tar.gz

After you've downloaded or built it, look at the files and if there are any, run the binaries:
ls -la /nix/store/spajxyizs6nqyx19rzpzlvkrmja3jysx-ccextractor-0.93
ls -la /nix/store/spajxyizs6nqyx19rzpzlvkrmja3jysx-ccextractor-0.93/bin

Pre-merge build results
We have automatically built all packages that will get rebuilt due to
this change.
This gives evidence on whether the upgrade will break dependent packages.
Note sometimes packages show up as failed to build independent of the
change, simply because they are already broken on the target branch.
Result of nixpkgs-review run on x86_64-linux 1

1 package built:

ccextractor

Maintainer pings
cc @titanous for testing.

Result of nixpkgs-review pr 134383 at 7ed3f15b run on aarch64-linux 1

1 package failed to build:

ccextractor

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.


StackTraceLayoutRenderer and StackTraceFormat.RawWithSource

Attempt to fix #2074

Codecov Report

Merging #2075 into master will increase coverage by <1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #2075    +/-   ##
=======================================
+ Coverage      81%     81%   +<1%
=======================================
  Files         289     289
  Lines       19902   19902
  Branches     2352    2353     +1
=======================================
+ Hits        16177   16191    +14
+ Misses       3126    3113    -13
+ Partials      599     598     -1

Thanks! I was thinking to create this, but you we're faster :)
PS: please do use the fixes syntax. Thanks!

updated docs for this: https://github.com/NLog/NLog/wiki/Stack-Trace-Layout-Renderer


opt_chocolatey_server_source variable gets truncated

I'm trying to use this role, but I keep getting an error when it is trying to get the chocolatey.server nupkg.
I set opt_chocolatey_server_source to a network path where the nupkg is located  \\Sources\chocolatey\server\  but strangely , when win_chocolatey is called in the "install the chocolatey.server package" section, it complains that it cannot find install.ps1 located at \\Sources\install.ps1   so it seems the variable gets truncated?
Edit:  Found the problem.  When "source" is supplied, the win_chocolatey.ps1 takes it and strip the end to try to access http:\sources\install.ps1to install the choco client.  But this will never work since the source path given is for a nupkg file, not a choco server.  It seems a functional choco server is required to install chocolatey.server??

closing this since it does not seems related to the role.

The fact that it is truncating the value to \\Sources\install.ps1 means that Chocolately is not installed on your host and it needs to find the Chocolatey install beforehand. There is a requirement set in Requirements section that says

Chocolatey client to be installed on the remote host if the remote host cannot access the internet

This is probably incorrect as the Chocolatey client (choco.exe) should already be installed if you are using opt_chocolatey_server_source at all (unless that is an existing Chocolatey server). It's unfortunately a bit of a Chicken vs egg situation but chocolatey.server is installed through Chocolatey hence that requirement.
I'll keep this issue open until I find time to clarify the docs a bit more and potentially add another variable which controls the source for Chocolatey itself separate from opt_chocolatey_server_source.


Manually adding XML declaration has no effect.

In the generateXML() function, this line
ET.Element('?xml version="1.0" encoding="UTF-8"?')
appears to have no effect as if it is its own separate tree. Remove it?

removed in justins commit.


Beta reduction should simplify lets the same as lambdas

let x = y in x
==>
y

not sure why we only do it when y is a value (https://github.com/ambiata/icicle/blob/master/icicle-data/src/Icicle/Common/Exp/Simp/Beta.hs#L38), gotta discuss with the big boss

Yeah, so variable should be fine, but anything else you might duplicate work. isSimpleValue (https://github.com/ambiata/icicle/blob/master/icicle-data/src/Icicle/Common/Exp/Simp/Beta.hs#L77) is really "is simple or value" rather than simple and value.
So maybe betaToLets should just substitute simple/values in.
Is this really a problem though?

It was just to improve the core output so it's easy to read.

Oh ok, cool

So is y a variable in this case, or an expression? For general expressions we'd want something like free variables freevars' :: Exp n -> Map n Count to see if it's used multiple times.
We might be able to get away with just matching on XLet x y (XVar x) though?

y is an expression and a pretty decent one at that, that's why it would be nice to reduce.
Yeah matching on XLet x y (XVar x) sounds like it would work I think.

@amos yeah I'm ok with matching on XLet x y (XVar x) (the conversion even has a name, so it's legit!)

Is that a specialised form of "zeta", or does it have its own name?

I think zeta is more general let x = e in e' --> e'[x := e] but I don't know. in a CBV semantics you would restrict the binding to a value, but I guess that would work in CBN?


Duplicate of "Use CLIException during explain command #779"

Summary

How to test

Checklist

[ ] I submit my changes into the develop branch
[ ] I have added description of my changes into CHANGELOG
[ ] I have updated the documentation accordingly
[ ] I have added tests to cover my changes
[ ] I have linked related issues

License

[ ] I submit my code changes under the same MIT License that covers the project.
Feel free to contact the maintainers if that's a concern.
[ ] I have updated the license header for each file (see an example below)

# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

Codecov Report
Base: 83.31% // Head: 83.29% // Decreases project coverage by -0.02% :warning:

Coverage data is based on head (0e7ca8c) compared to base (636158c).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                      Coverage Diff                      @@
##           releases/v0.4.0-geti1.1.0     #780      +/-   ##
=============================================================
- Coverage                      83.31%   83.29%   -0.03%
=============================================================
  Files                            166      166
  Lines                          22432    22434       +2
  Branches                        4895     4896       +1
=============================================================
- Hits                           18690    18686       -4
- Misses                          2637     2642       +5
- Partials                        1105     1106       +1

Flag
Coverage Δ

macos-10.15_Python-3.10
?

macos-10.15_Python-3.7
?

macos-10.15_Python-3.8
?

macos-10.15_Python-3.9
?

ubuntu-20.04_Python-3.10
83.24% <50.00%> (-0.01%)
:arrow_down:

ubuntu-20.04_Python-3.7
82.91% <50.00%> (-0.01%)
:arrow_down:

ubuntu-20.04_Python-3.8
83.17% <50.00%> (-0.01%)
:arrow_down:

ubuntu-20.04_Python-3.9
83.18% <50.00%> (-0.01%)
:arrow_down:

windows-2019_Python-3.10
?

windows-2019_Python-3.7
?

windows-2019_Python-3.8
?

windows-2019_Python-3.9
?

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

Impacted Files
Coverage Δ

datumaro/cli/commands/explain.py
35.55% <50.00%> (+0.32%)
:arrow_up:

datumaro/util/os_util.py
75.75% <0.00%> (-1.52%)
:arrow_down:

datumaro/util/test_utils.py
88.39% <0.00%> (-1.11%)
:arrow_down:

datumaro/plugins/splitter.py
88.47% <0.00%> (-0.23%)
:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


Fix tests

This re-enables most of the tests.

Back ported to hydro: https://github.com/ros-visualization/rviz/pull/900


[Hold] Notification received for sent and deleted message in offline

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:

Open a chat on desktop
Go offline
Send a message and delete the message
Go online
Open android app on the receiving account
Wait a couple of seconds and observe the notification panel

Expected Result:
The deleted message should not appear on the notification panel on android.
Actual Result:
The deleted message appears on the notification panel on the receiving account’s phone and its content can be seen.
Workaround:
unknown
Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.2.97-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/43996225/230750536-cd0cd5a1-3777-4421-ae11-25ebb97b9ac6.mp4
https://user-images.githubusercontent.com/43996225/230750537-cb65b1fb-303a-414c-ad21-95a38dc894e7.mp4

Expensify/Expensify Issue URL:
Issue reported by:  @Nathan-Mulugeta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1680959362130529
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~019363c7307b2d17b5
Upwork Job ID:<PHONE_NUMBER>844620288
Last Price Increase: 2023-04-13

Triggered auto assignment to @arielgreen (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)

If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead

[ ] This bug is reproducible using the reproduction steps in the OP. S/O

If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
If the reproduction steps aren't clear and you determine the correct steps, please update the OP.

[ ] This issue is filled out as thoroughly and clearly as possible

Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.

[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Confirmed reproducible.

Job added to Upwork: https://www.upwork.com/jobs/~019363c7307b2d17b5

Triggered auto assignment to Contributor Plus for review of internal employee PR - @s77rt (Internal)

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@bondydaa I am like 98% sure notifications issues like this need to be internal -- can you confirm?

probably since this is most likely dealing with Pusher events. I just learned about this tracking issue https://github.com/Expensify/App/issues/12775? which is dealing with "replay effect" of actions which I think is probably the underlying issue here.

let's hold this for https://github.com/Expensify/App/issues/12775 as well

@bondydaa I don't think the replay effect will fix this, because the server will still receive an AddComment request and an EditComment request to delete the message, so we will still send the notification when adding the comment. I think that's fine actually, it's equivalent to adding and deleting a comment quickly while online.

hmm gotcha. so are you suggesting then that we close this out?

Yeah I would close this. Someday in the future we might build request de-duplication, which would fix this, but so far no one has proposed anything and I think it's pretty complicated and low ROI.

:woohoo: I love :do-nothing:


Avoid -t option in mktemp

The semantic of -t depends on the various mktemp implementations...  Avoid possibly workaround-ing/adjusting that and instead just use it without any arguments.

This should hopefully fix a regression in #50 that was not spotted because tests currently do not run!

I'm bypassing the branch protections again to hopefully unblock #51.


Adds script to help craft release notes

Implements #7581
Example Usage:
$ PREVIOUS_RELEASE=v3.0.0 make release-notes
## v3.1.0

Helm v3.1.0 is a feature release. This release, we focused on <insert focal point>. Users are encouraged to upgrade for the best experience.

The community keeps growing, and we'd love to see you there!

- Join the discussion in [Kubernetes Slack](https://kubernetes.slack.com):
  -  for questions and just to hang out
  -  for discussing PRs, code, and bugs
- Hang out at the Public Developer Call: Thursday, 9:30 Pacific via [Zoom](https://zoom.us/j/696660622)
- Test, debug, and contribute charts: [GitHub/helm/charts](https://github.com/helm/charts)

## Notable Changes

- Add list of
- notable changes here

## Installation and Upgrading

Download Helm v3.1.0. The common platform binaries are here:

- [MacOS amd64](https://get.helm.sh/helm-v3.1.0-darwin-amd64.tar.gz) ([checksum](https://get.helm.sh/helm-v3.1.0-darwin-amd64.tar.gz.sha256) / aacb6ce8ffa08eebc4e4a570226675f53963c86feb8386d46abf4b8871066c92)
- [Linux amd64](https://get.helm.sh/helm-v3.1.0-linux-amd64.tar.gz) ([checksum](https://get.helm.sh/helm-v3.1.0-linux-amd64.tar.gz.sha256) / f0fd9fe2b0e09dc9ed190239fce892a468cbb0a2a8fffb9fe846f893c8fd09de)
- [Linux arm](https://get.helm.sh/helm-v3.1.0-linux-arm.tar.gz) ([checksum](https://get.helm.sh/helm-v3.1.0-linux-arm.tar.gz.sha256) / cb2824c01860196fab8cd6ececd04ff78e9c6606d175e6cd5f41e7d99881795b)
- [Linux arm64](https://get.helm.sh/helm-v3.1.0-linux-arm64.tar.gz) ([checksum](https://get.helm.sh/helm-v3.1.0-linux-arm64.tar.gz.sha256) / 1ba32db0600db61d8ace7a3afbaf7b045e16c0aab2a054dd9ec8e02755c07674)
- [Linux i386](https://get.helm.sh/helm-v3.1.0-linux-386.tar.gz) ([checksum](https://get.helm.sh/helm-v3.1.0-linux-386.tar.gz.sha256) / 9bb03099968f16c20298773fe5e466fa66206bf0f125ea656e1722cd86f32439)
- [Linux ppc64le](https://get.helm.sh/helm-v3.1.0-linux-ppc64le.tar.gz) ([checksum](https://get.helm.sh/helm-v3.1.0-linux-ppc64le.tar.gz.sha256) / 6ba4a2a6690c0224ab513971e6c56243c60ffbcc3ba4f68960a693b070bd5f71)
- [Linux s390x](https://get.helm.sh/helm-v3.1.0-linux-s390x.tar.gz) ([checksum](https://get.helm.sh/helm-v3.1.0-linux-s390x.tar.gz.sha256) / aacb6ce8ffa08eebc4e4a570226675f53963c86feb8386d46abf4b8871066c92)
- [Windows amd64](https://get.helm.sh/helm-v3.1.0-windows-amd64.zip) ([checksum](https://get.helm.sh/helm-v3.1.0-windows-amd64.zip.sha256) / f6a6ee20bd216beb2f0195f083b03d43b8801e885a483011807824bd0915b835)

The [Quickstart Guide](https://docs.helm.sh/using_helm/#quickstart-guide) will get you going from there. For **upgrade instructions** or detailed installation notes, check the [install guide](https://docs.helm.sh/using_helm/#installing-helm). You can also use a [script to install](https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3) on any system with `bash`.

## What's Next

- 3.1.1 will contain only bug fixes.
- 3.2.0 is the next feature release. This release will focus on ...

## Changelog

- add test for template recursion b29d20baf09943e134c2fa5e1e1cab3bf93315fa (Daniel Cheng)
- fix recursion count in templates 805a591b5057845be0e872a3f667e81d07203367 (Daniel Cheng)
- Fix shasums to be usable by shasum and sha256sum applications 9887a3e4ca4ef473a4ddd419573edbd5c82bb231 (Matt Farina)
...
...
- Porting fix from commit f5986db184cf6d16dcd48760ac749a20236fb845 afda6b49408bad8f7a7a5faa9eeda016c8763400 (Lam Le)
- Fix import 41e70306b3464b99cf3afe4e23e85948fede3483 (Yagnesh Mistry)
- fix rename for helm dependency upgrade ceb6bcb318fa1f93ad2f67e3ba1cbbf1600fc154 (Yagnesh Mistry)

Signed-off-by: Paul Czarkowski<EMAIL_ADDRESS>

PS, out of scope for this PR, but have we discussed using https://goreleaser.com/ and customize the release notes template etc?

goreleaser controls the entire release workflow - which is nice for new projects - but it doesn't match up with our current release process. I don't see a way where we could use it given our current requirements.
That being said, I have been playing with Github Actions recently to help replace some of our existing processes. I built Azure Blob Storage Upload to handle some of the cases CircleCI handles in our pipeline today, for example.
We could look at writing more Github Actions to automate the release process, including what @paulczar's proposing here.

@paulczar if you want to add this to the release checklist, that'd be great. That way this script would be discovered by others going through the release process.
https://github.com/helm/helm-www/blob/master/content/en/docs/community/release_checklist.md
Ok, merging now. Thanks!


Deleting empty folders recursively, skipping hidden directories

I've been using this for a while and it works well
bfs -type d -exec bfs -f {} -not -type d -exit 1 \; -prune -ok bfs -f {} -type d -delete \;

How do I skip top-level hidden folders? I tried this
bfs -nohidden -type d -exec bfs -nohidden -f {} -not -type d -exit 1 \; -prune -ok bfs -nohidden -f {} -type d -delete \;

but it seems to match more folders
bfs: error: ./some/folder: Directory not empty.

I think you have too many -nohiddens :)

The first one causes the outer bfs to skip hidden files, so they won't be checked for emptiness or deleted.  I think this is what you want.
The second one causes bfs to ignore hidden files when checking if a directory is empty.  Maybe this is also what you want?
The third one causes bfs to ignore hidden files when deleting.  This is probably not what you want, since if you don't delete a hidden file, you can't delete its parent either, etc.

yes you are right. The bfs -nohidden -type d -exec bfs -f {} -not -type d -exit 1 \; -prune -ok bfs -f {} -type d -delete \; incantation seems to be the desired behavior

Circling back to this, I actually had the same desire to delete "recursively empty" directories the other day, and it occurred to me that the following simpler invocation works:
tavianator@graphene $ bfs -type d -empty -delete

This works because -delete turns on post-order traversal (aka -depth), so it will start with leaf directories and work upwards.  At each level, -empty is testing the directory after we've already deleted its empty children.
For your use case, I believe just adding -nohidden does the right thing, but I didn't think too carefully about it.  Here's an example though:
tavianator@graphene $ tree -a
.
├── empty
├── full
│   └── file
├── .hidden
│   └── empty
└── recursively
    └── empty

7 directories, 1 file
tavianator@graphene $ bfs -nohidden -type d -empty -delete
tavianator@graphene $ tree -a
.
├── full
│   └── file
└── .hidden
    └── empty

4 directories, 1 file

You do lose the -ok confirmation this way.  You could do
tavianator@graphene $ bfs -depth -nohidden -type d -empty -ok rmdir {} \;
rmdir ./empty ? y
rmdir ./recursively/empty ? y
rmdir ./recursively ? y

But that prompts on every deleted directory, not just the top-level ones.


python3Packages.pydy: fix build

Description of changes
fix build (https://hydra.nixos.org/build/259534879)
ZHF: #309482
pydy tests require nose which not support python 3.10 or later

Things done

Built on platform(s)

[ ] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[ ] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[ ] Tested basic functionality of all binary files (usually in ./result/bin/)
24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[ ] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

Result of nixpkgs-review pr 313627 run on x86_64-linux 1

4 packages built:

python311Packages.pydy
python311Packages.pydy.dist
python312Packages.pydy
python312Packages.pydy.dist

Result of nixpkgs-review pr 313627 run on aarch64-darwin 1

4 packages built:

python311Packages.pydy
python311Packages.pydy.dist
python312Packages.pydy
python312Packages.pydy.dist


tail: fix argument parsing of sleep interval

Like discussed, I've extracted this from the refactoring pr of tail, which fixes the parsing of the sleep interval.
The main intend of this pr is to match the behavior of gnu's tail and properly parse a string in f64 format to a Duration.
Short summary:

The sleep interval is a f64 instead of a f32
Duration::from_secs_f64 panics and Duration::try_from_secs_f64 is not stable, so we need a separate function to accomplish the conversion from f64 to a Duration.

How does this parse_duration() function relate to uucore::parse_time::from_str()? https://github.com/uutils/coreutils/blob/bdd6f56ae4e88c38190c6780ef6b5594996ee03d/src/uucore/src/lib/parser/parse_time.rs#L17-L49

Not sure what you mean, but they don't relate I think. This function tries to provide the same functionality like gnu's tail parsing of the --sleep-interval flag. So, there are for example no units like s, ms etc. allowed and if the number on the command line overflows f64 there's an error. That's the main differences to the uucore::parse_time::from_str function as far as I can see.

Please do not merge. I'm just trying something, to get a more performant, precise and unlossy version of the parse_duration method.

@tertsdiepraam I've completely rewritten the parse_duration method and moved it because of its size into the parse module of tail.
This duration parser is almost as fast as Duration::from_secs_f64(input.parse::<f64>().unwrap), has the same grammar like a f64 parser but is not lossy (due to rounding and lack of floating point precision) by storing the exact digit representation of the input string. parse_duration is around 2 - 5 times slower depending on the size of the input string but still operates in the nano seconds domain and takes around 100 - 300 ns for normal input like 1.0e2 on my system. More extreme input like format!("{}.{}e1023", "1".repeat(2000), "9".repeat(2000)) which can't even parsed without errors by the Duration::from_secs_f64 method still ran in around 5 microseconds on my quadcore system.
If the seconds overflow u64::MAX, the duration is interpreted as Duration::MAX, which is also an improvement over gnu's tail, I think. Imho we don't need to replace the parse_duration method with Duration::try_from_secs_f64 anymore. What do you think?

I'll have to look closely into the code later, but it sounds great! It does add significant complexity to the code over a single function call, but the advantages you list might be worth it.
Unrelated to the actual change, but only to the tests (which I only quickly skimmed), I'm not sure I like using rstest for creating multiple test cases. Personally, I think a for loop is easier to read and more approachable for new contributors.

Ok, cool. Parameterized tests have the advantage that all test cases can run (and fail). A for loop stops at the first error encountered and it's maybe not clear which case actually failed. That's even worse in the CI, because we don't output the backtrace there. Parameterizing the tests helped me a lot during the writing. I actually don't think this is a huge obstacle for new contributors, since the syntax is quiet clear #[case::some_descriptive_name(...)] (even without knowing anything about rstest) and the output of an errored test case is parse::tests::my_test::case_1_some_descriptive_name pointing to the exact test case which went south by also providing a first hint. rstest is also well documented in my opinion, on one page in the github README and on docs.rs.

I'll have to look closely into the code later,

Have you looked at this yet?

I had the idea to move this into uucore, if this is too big for tail alone. It's fairly simple to add time unit parsing and make the time units customizable. Each uutil may have its own set of time units it accepts. Maybe we can do this once and for all uutils by also providing a speedy and precise parsing of a Duration. If required, I had some additional ideas to max out the parsing speed, but this would also add additional complexity. What do you think?

No offense, but I extracted the code I've written into an own crate https://crates.io/crates/fundu. The idea and much of the code is the same. I've taken care, that it's compatible with the requirements of uutils :) I don't intend to change that and I hope you see this as an advantage over the original post. It is also a lot of lines of code less in uutils. Would be happy to hear your opinion and maybe see fundu used in uutils.

@tertsdiepraam ping?

I'll take a look now :)

Alright, I think this is good enough on the uutils side. Excellent work on the crate too!
I think it would be interesting to show benchmarks against simple String -> f64 -> Duration to provide some proof that it really has a low (or no) overhead. You could also show some examples where that fails and where fundu succeeds in the README. You could also document a bit more clearly what the default units are. On the code side, the only thing I have to comment, is that the multiplier is a bit confusing because it's sometimes 10^-m and sometimes a normal multiplier. Why not split that into two functions or maybe a function that returns a multiplier and an exponent in a tuple? That might also clean up some branching.

Thanks for your review. It's much appreciated.

I think it would be interesting to show benchmarks against simple String -> f64 -> Duration to provide some proof that it really has a low (or no) overhead

good idea. Currently, the comparison with String -> f64 -> Duration can be seen when running the benches. It's the reference function. fundu will always have some (low) overhead compared to Duration::from_secs_f64 combined with f64::from_str because of its precision and the integration of time unit parsing etc. Parsing some simple input with fundu takes around 50ns and the stdlib methods combined around 26ns on my testing machine. I think, in most use cases this difference won't be noticable. However, I'm working on lowering the difference by some additional nano seconds (maybe 5ns or so) for small input, but especially for large input.

You could also show some examples where that fails and where fundu succeeds in the README. You could also document a bit more clearly what the default units are.

yeah that would be good. It'll be part of 0.3.0. I added a lot of documentation for the public api in general there.

On the code side, the only thing I have to comment, is that the multiplier is a bit confusing because it's sometimes 10^-m and sometimes a normal multiplier. Why not split that into two functions or maybe a function that returns a multiplier and an exponent in a tuple? That might also clean up some branching.

you're right it's confusing. Do you want that method pub? However, I'll try out the tuple.

Do you want that method pub?

Not really, I was just checking out the code and this stood out :)

ok :) I think this method is not really useful outside of the crate, so I'll remove it from the public api for now.

Are we done here?

We made a change to how we declare dependencies so the Cargo.toml conflicts, but apart from it's good!

ok great! The errors in the ci aren't related to this pr as far as I can see. However, I'm triggering a rerun, so maybe the deny step recovers.

ok, looks like the deny step recovered.

Thanks :)


Duplications due to mixed case "Content Description" values

The "Content Description" facet in the Data Browser "File" menu (file_core.content_description.ontology_label/text) contains duplications due to the use of mixed case:

"annotated text" vs "Annotated text"
"Count Matrix" vs "Count matrix"
"DNA sequence" vs "DNA sequence (raw)" { is the added " (raw)" necessary?]

Acceptance criteria for the task:

standardize on case usage to remove duplications

Moving to our backlog for addressing when prioritised


Import Data.Monoid.<> when necessary

This is an attempt to fix https://github.com/joshuaclayton/unused/issues/64


Designer PDF viewer

I have solved this using python
Based on #124

LGTM , Thank you for the contribution.


README: use SVG badges

This PR changes the build badges to SVG for sharper readability ✨

Cheers!