Skip to content

Conversation

@zeusongit
Copy link
Contributor

Purpose

This is a separate branch that will include all the visual changes until the backend is wired up, that includes a way to populate package dependency field with Python versions used in that package.

pyspy3

@zeusongit zeusongit requested a review from QilongTang April 14, 2021 15:38
@Amoursol
Copy link
Contributor

The UX implementation and colors LGTM @zeusongit !

@QilongTang
Copy link
Contributor

@zeusongit So this PR is about using dependency field. Is there a different PR using the HostDependency implementation for us to compare?

//added to this dummy package for representation, to be removed and replace space at the end with newline above
if (Name == "dronovBIM_v2")
{
hostsString += "CPython3"+ Environment.NewLine + "IronPython2"+ Environment.NewLine + "Revit";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QilongTang Here HostsString is derived from host_dependencies that we are using to display dependencies, including python deps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid string literals wherever possible. Makes it easy to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wont be part of the final version and will be removed before it is merged

element.Versions[0].Item1.full_dependency_ids.Add(new Dependency() { name = "CPython", _id = "3.0" });
element.Versions[1].Item1.full_dependency_ids.Add(new Dependency() { name = "IronPython", _id = "2.0" });
element.Versions[2].Item1.full_dependency_ids.Add(new Dependency() { name = "IronPython", _id = "2.0" });
element.Versions[2].Item1.full_dependency_ids.Add(new Dependency() { name = "CPython", _id = "3.0" });
Copy link
Contributor Author

@zeusongit zeusongit Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add another property such as version or vers to record version instead of _id from PMClient class while implementing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i think version would be a better choice.

@zeusongit
Copy link
Contributor Author

I would like to conclude this spike, so that we can move further with discussion/implementation on this topic.
In this PR i have utilised both the approaches i.e to use host_dependencies as well as full_dependency_ids to display information.

Screen Shot 2021-04-16 at 4 22 59 PM

There are some observations/concerns:

  • If using 'host_dependencies' we lose version info.
  • Do we display the latest versions dependency as the primary dependency in the hyperlink tooltip? Or a cumulative list of dependencies from all versions?
  • If using 'full_dependency_ids' we need to figure out a way to populate exact versions of dependencies.
  • Also we need to provide a way for these properties to be set, by the user or by some implicit process while publishing a new package.
  • There also has to be a one-time function that scans all the packages and fill in dependency data for the existing packages.

@reddyashish
Copy link
Contributor

@zeusongit just one more comment, forgot to mention it earlier. LGTM. What about the dummy values added in the PR? Will they be removed once the followup changes are made? If yes, please make sure the next task mentions that.

@zeusongit
Copy link
Contributor Author

@zeusongit just one more comment, forgot to mention it earlier. LGTM. What about the dummy values added in the PR? Will they be removed once the followup changes are made? If yes, please make sure the next task mentions that.

Yes @reddyashish the dummy values will be removed, if you see the base branch of the PR that is not master, this will be merged in a separate branch until other parts of this task are worked on.

@reddyashish reddyashish added the LGTM Looks good to me label Apr 21, 2021
@zeusongit zeusongit merged commit cf0fae1 into DynamoDS:pydep-spike Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM Looks good to me

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants