Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove old "edit" and "search" commands. #4291

Merged
merged 4 commits into from
Jul 7, 2021

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 5, 2021

This is a small change with no associated Issue - discussed on Element chat.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.py and conda-environment.yml.
  • Does not need tests (Removed code and tests).
  • Appropriate change log entry included.
  • (master branch) I have opened a documentation PR at Remove references to obsolete "cylc edit" command. cylc-doc#261

@hjoliver hjoliver force-pushed the remove-cylc-edit-cylc-diff branch from 867530b to 1fabdab Compare July 5, 2021 23:31
@hjoliver
Copy link
Member Author

hjoliver commented Jul 5, 2021

Should we remove cylc diff too?

$ cylc diff --help
Usage: cylc diff [OPTIONS] WORKFLOW1 WORKFLOW2

Compare two workflow configurations and display any differences.

Differencing is done after parsing the flow.cylc files so it takes
account of default values that are not explicitly defined, it disregards
the order of configuration items, and it sees any include-file content
after inlining has occurred.

Files in the workflow bin directory and other sub-directories of the
run directory are not currently differenced.

I suppose this could be useful as described. The diffs are presented after parsing so order of sections doesn't matter. Or shall we just tell users to go to standard diff (perhaps with flow.cylc.processed to see diffs after inlining and Jinja2 processing)?

@hjoliver hjoliver requested a review from wxtim July 5, 2021 23:55
@hjoliver
Copy link
Member Author

hjoliver commented Jul 5, 2021

And similarly, cylc search?


Usage: cylc search [OPTIONS] WORKFLOW PATTERN [PATTERN2...]

Search for patterns in workflow configurations.

Search for pattern matches in workflow definitions and any files in the
workflow bin directory. Matches are reported by line number and workflow
section. An unquoted list of PATTERNs will be converted to an OR'd
pattern. Note that the order of command line arguments conforms to
normal cylc command usage (workflow name first) not that of the grep
command.

Note that this command performs a text search on the workflow definition,
it does not search the data structure that results from parsing the
workflow definition - so it will not report implicit default settings.

For case insensitive matching use '(?i)PATTERN'.

@hjoliver
Copy link
Member Author

hjoliver commented Jul 5, 2021

I'm not sure the benefits are sufficient to bother (with cylc diff and cylc search) c.f. standard diff and grep.

@wxtim
Copy link
Member

wxtim commented Jul 6, 2021

I'm not sure the benefits are sufficient to bother (with cylc diff and cylc search) c.f. standard diff and grep.

Cylc diff - Looks like it does some useful extra stuff. I can see uses for it in terms of convincing customers that "yes, they have changed their workflow"

Cylc search - Don't see the point

@oliver-sanders
Copy link
Member

Yeah, same opinion here:

  • edit use a text editor.
  • search use grep or a text editor.

However, diff and graph --diff are somewhat different as they compare the parsed config. They wouldn't be worth the effort of implementing now, however, since they still work and require little maintenance there is little impetus to remove them.

@hjoliver hjoliver changed the title Remove old "cylc edit" command. Remove old "edit" and "search" commands. Jul 6, 2021
@hjoliver
Copy link
Member Author

hjoliver commented Jul 6, 2021

Roger that. Updated to remove cylc search as well as cylc edit. Retaining cylc diff.

@hjoliver
Copy link
Member Author

hjoliver commented Jul 6, 2021

One review will do.

@kinow
Copy link
Member

kinow commented Jul 7, 2021

CI failure appears to be unrelated

=================================== FAILURES ===================================
_____________________________ test_delta_task_held _____________________________
[gw4] linux -- Python 3.8.10 /opt/hostedtoolcache/Python/3.8.10/x64/bin/python

harness = (Scheduler(profiler=<cylc.flow.profiler.Profiler object at 0x7f3e0dd0b4c0>, pool=<cylc.flow.task_pool.TaskPool object ...e: 2
}
is_held_total: 0
is_queued: false
is_queued_total: 0
is_runahead: true
is_runahead_total: 1
}, 'jobs': {}, ...})

    def test_delta_task_held(harness):
        """Test update_data_structure. This method will generate and
        apply adeltas/updates given."""
        schd, data = harness
>       assert True in {t.is_held for t in data[TASK_PROXIES].values()}
E       assert True in {False}

tests/integration/test_data_store_mgr.py:191: AssertionError
__________________________ test_update_data_structure __________________________
[gw4] linux -- Python 3.8.10 /opt/hostedtoolcache/Python/3.8.10/x64/bin/python

harness = (Scheduler(profiler=<cylc.flow.profiler.Profiler object at 0x7f3e0dd0b4c0>, pool=<cylc.flow.task_pool.TaskPool object ...e: 2
}
is_held_total: 0
is_queued: false
is_queued_total: 0
is_runahead: true
is_runahead_total: 1
}, 'jobs': {}, ...})

    def test_update_data_structure(harness):
        """Test update_data_structure. This method will generate and
        apply adeltas/updates given."""
        schd, data = harness
        w_id = schd.data_store_mgr.workflow_id
        schd.data_store_mgr.data[w_id] = data
        assert TASK_STATUS_FAILED not in set(collect_states(data, TASK_PROXIES))
        assert TASK_STATUS_FAILED not in set(collect_states(data, FAMILY_PROXIES))
        assert TASK_STATUS_FAILED not in data[WORKFLOW].state_totals
>       assert len({t.is_held for t in data[TASK_PROXIES].values()}) == 2
E       assert 1 == 2
E         +1
E         -2

Opened the IDE and searched for other parts using edit and found nothing, except cat-log that appears to support an edit mode, but I think that's not related to this edit command that was removed.

LGTM 🎉

@kinow kinow merged commit d26bf20 into cylc:master Jul 7, 2021
@hjoliver hjoliver deleted the remove-cylc-edit-cylc-diff branch July 7, 2021 02:03
@oliver-sanders
Copy link
Member

I think those failures are caused by this issue - #4175

These tests are about 50/50 on my machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants