fossasia / visdom

A flexible tool for creating, organizing, and sharing visualizations of live, rich data. Supports Torch and Numpy.
Apache License 2.0
9.93k stars 1.13k forks source link

Merge variables envID and envIDs #914

Closed da-h closed 1 year ago

da-h commented 1 year ago

Description

Just a small proposal for simplification. In the react component (js/main.js), there is a variable called envID and another one called envIDs. As envID == envIDs[0] if any env is selected, this PR suggests to remove the duplicate variable envID.

Motivation and Context

The javascript code distinguishes the case of a single selected environment and multiple ones. However, if multiple ones are selected, some functions effectively use just the first environment of the list, instead of merging the environments somehow. For instance, forking environments or views is also possible for multiple envs selected, in both cases it uses always the name of the first environment. (I guess this is minor a bug).

I know that the proposed change does not solve this at all, still I suggest to simplify the code at this point. If you have an idea how to improve this PR, please let me know. :) For instance, I think it would be reasonable to just disable the Fork- and View-Buttons when multiple envs are selected for now as their behavior is currently not well-defined.

How Has This Been Tested?

Just a short local test.

Types of changes

Checklist:

da-h commented 1 year ago

Refactoring something like this is easier than writing it new from scratch, when the function set of the project was growing, and it was unclear where the project was heading.