mattcasters commented on code in PR #8553:
URL: https://github.com/apache/hop/pull/8553#discussion_r4083600833
##########
plugins/misc/projects/src/main/java/org/apache/hop/projects/config/ProjectsConfigOptionPlugin.java:
##########
@@ -287,13 +284,9 @@ public boolean handleOption(
changed = true;
}
if (environmentsForActiveProject != null) {
- config.setEnvironmentsForActiveProject(environmentsForActiveProject);
- if (environmentsForActiveProject) {
- log.logBasic("Only listing environments for the active project");
- } else {
- log.logBasic("Listing all environments, regardless of the active
project");
- }
- changed = true;
+ log.logBasic(
+ "Option --environments-for-active-project is deprecated and has no
effect: the"
Review Comment:
**[suggestion]** This notice (and the new field javadoc) says the
environment list is always restricted to the active project.
`ProjectsGuiPlugin.createEnvironmentContextMenu` also lists environments whose
project name is empty, and the manual sentence added in this PR documents that.
Align the log and the javadoc with that rule so the deprecation text does not
contradict the docs.
**Suggestion:** Say the menu lists the active project's environments and
environments that are not linked to any project.
##########
plugins/misc/projects/src/main/java/org/apache/hop/projects/config/ProjectsConfigOptionPlugin.java:
##########
@@ -287,13 +284,9 @@ public boolean handleOption(
changed = true;
}
if (environmentsForActiveProject != null) {
- config.setEnvironmentsForActiveProject(environmentsForActiveProject);
- if (environmentsForActiveProject) {
- log.logBasic("Only listing environments for the active project");
- } else {
- log.logBasic("Listing all environments, regardless of the active
project");
- }
- changed = true;
+ log.logBasic(
Review Comment:
**[bug]** This branch no longer sets `changed`, so `handleOption` returns
false when `-eap` / `--environments-for-active-project` is the only option.
`HopConfigBase.run()` treats a false result from every mixin as "no action" and
prints full usage on stdout (`if (!actionTaken) { cmd.usage(System.out); }`).
The process still exits 0, but a script that only passes this flag now looks
unhandled. That undercuts the comment above, which keeps the option so existing
scripts keep working. Other flags in the same invocation still save, because
they set `changed` themselves.
**Suggestion:** Do not persist the flag, but treat it as handled: `return
changed || environmentsForActiveProject != null` (do not return early, or later
options in this method are skipped). Cover that with a test that calls
`handleOption` and asserts true without writing `environmentsForActiveProject`
back into the config.
##########
plugins/misc/projects/src/main/java/org/apache/hop/projects/config/ProjectsConfigOptionPlugin.java:
##########
@@ -163,15 +161,15 @@ public class ProjectsConfigOptionPlugin
description = "The project configuration filename for new projects")
private String defaultProjectConfigFile;
- @GuiWidgetElement(
- id = WIDGET_ID_RESTRICT_ENVIRONMENTS_TO_ACTIVE_PROJECT,
- parentId = ConfigPluginOptionsTab.GUI_WIDGETS_PARENT_ID,
- type = GuiElementType.CHECKBOX,
- variables = false,
- label = "i18n::ProjectConfig.RestrictEnvsToActiveProject.Message")
+ /**
+ * No longer has any effect: since 2.17 the environment menu in the status
bar always lists only
+ * the environments of the active project. The option is still accepted so
that existing hop-conf
+ * scripts keep working.
Review Comment:
**[nit]** The comment says `hop-conf`. The tool is `hop-config`.
**Suggestion:** Replace `hop-conf` with `hop-config`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
