Skip to content

🔥 DOP-4290 removes Manifest.py and Document.py as part of the deprecating of the legacy tooling support#71

Merged
caesarbell merged 6 commits into
mainfrom
DOP-4290
Feb 8, 2024
Merged

🔥 DOP-4290 removes Manifest.py and Document.py as part of the deprecating of the legacy tooling support#71
caesarbell merged 6 commits into
mainfrom
DOP-4290

Conversation

@caesarbell
Copy link
Copy Markdown
Contributor

No description provided.

@caesarbell caesarbell marked this pull request as ready for review February 6, 2024 20:06
Copy link
Copy Markdown
Collaborator

@anabellabuckvar anabellabuckvar left a comment

Choose a reason for hiding this comment

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

Thanks for this! README looks great. Two notes

  1. I think there are a few dependencies that were only used in Document.py and/or Manifest.py, such as html5_parser and lxml(but possibly more). What do you think about checking and removing such dependencies from setup.py and pyroject.toml?
  2. I could be wrong, but it seems like there is still some code for legacy tooling that uses giza/sphinx and deals with building manifests from an html document. I noticed this in convert_redirects, for example, but please ignore if you already saw that code and decided to leave it in!

@caesarbell
Copy link
Copy Markdown
Contributor Author

Thanks for this! README looks great. Two notes

  1. I think there are a few dependencies that were only used in Document.py and/or Manifest.py, such as html5_parser and lxml(but possibly more). What do you think about checking and removing such dependencies from setup.py and pyroject.toml?
  2. I could be wrong, but it seems like there is still some code for legacy tooling that uses giza/sphinx and deals with building manifests from an html document. I noticed this in convert_redirects, for example, but please ignore if you already saw that code and decided to leave it in!

@anabellabuckvar good catch, I also removed it from those dependencies from requirement.txt as well. As for the convert_redirects.py, how can I confirm that this was used for legacy tooling?

@anabellabuckvar
Copy link
Copy Markdown
Collaborator

Thanks for this! README looks great. Two notes

  1. I think there are a few dependencies that were only used in Document.py and/or Manifest.py, such as html5_parser and lxml(but possibly more). What do you think about checking and removing such dependencies from setup.py and pyroject.toml?
  2. I could be wrong, but it seems like there is still some code for legacy tooling that uses giza/sphinx and deals with building manifests from an html document. I noticed this in convert_redirects, for example, but please ignore if you already saw that code and decided to leave it in!

@anabellabuckvar good catch, I also removed it from those dependencies from requirement.txt as well. As for the convert_redirects.py, how can I confirm that this was used for legacy tooling?

I'm not sure, it mentions giza-style redirects and versions, but this will likely need further investigation or others on the team might have insight as to whether those code components are still necessary

@caesarbell
Copy link
Copy Markdown
Contributor Author

Thanks for this! README looks great. Two notes

  1. I think there are a few dependencies that were only used in Document.py and/or Manifest.py, such as html5_parser and lxml(but possibly more). What do you think about checking and removing such dependencies from setup.py and pyroject.toml?
  2. I could be wrong, but it seems like there is still some code for legacy tooling that uses giza/sphinx and deals with building manifests from an html document. I noticed this in convert_redirects, for example, but please ignore if you already saw that code and decided to leave it in!

@anabellabuckvar good catch, I also removed it from those dependencies from requirement.txt as well. As for the convert_redirects.py, how can I confirm that this was used for legacy tooling?

I'm not sure, it mentions giza-style redirects and versions, but this will likely need further investigation or others on the team might have insight as to whether those code components are still necessary

@anabellabuckvar, fair, so for learning purposes, giza/sphinx technology was only used for legacy tooling? I will raise with the team our question about convert_redirects being solely used for legacy tooling.

Copy link
Copy Markdown
Collaborator

@anabellabuckvar anabellabuckvar left a comment

Choose a reason for hiding this comment

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

Looks good! Last thing is make sure to remove the convert-redirects script and any references to it

@anabellabuckvar
Copy link
Copy Markdown
Collaborator

Thanks for this! README looks great. Two notes

  1. I think there are a few dependencies that were only used in Document.py and/or Manifest.py, such as html5_parser and lxml(but possibly more). What do you think about checking and removing such dependencies from setup.py and pyroject.toml?
  2. I could be wrong, but it seems like there is still some code for legacy tooling that uses giza/sphinx and deals with building manifests from an html document. I noticed this in convert_redirects, for example, but please ignore if you already saw that code and decided to leave it in!

@anabellabuckvar good catch, I also removed it from those dependencies from requirement.txt as well. As for the convert_redirects.py, how can I confirm that this was used for legacy tooling?

I'm not sure, it mentions giza-style redirects and versions, but this will likely need further investigation or others on the team might have insight as to whether those code components are still necessary

@anabellabuckvar, fair, so for learning purposes, giza/sphinx technology was only used for legacy tooling? I will raise with the team our question about convert_redirects being solely used for legacy tooling.

AFAIK, yes, those are legacy and we will no longer be supporting that tooling

Copy link
Copy Markdown
Collaborator

@anabellabuckvar anabellabuckvar left a comment

Choose a reason for hiding this comment

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

LGTM :)

@caesarbell caesarbell merged commit b87323d into main Feb 8, 2024
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.

2 participants