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

Enabled flexible use of URLs #228

Closed
wants to merge 15 commits into from
Closed

Enabled flexible use of URLs #228

wants to merge 15 commits into from

Conversation

lscheffler
Copy link
Contributor

@lscheffler lscheffler commented Dec 1, 2023

Solving #225.

  • Added DEFINEs to allow centralized input of repo and branch.
  • Altered respective code
  • Altered Changlog

See CONTRIBUTING what files are touched and how to use.

Download https://github.com/lscheffler/Thor/raw/dev/ThorUpdater/Thor.zip to create a test installation running from the dev branch of the lscheffler fork.

Update:
It is intended that there is only a little number of modules in the dev branch. This should not harm any other branch.

Added Ed Raus API_Run class
Follow up changes since 20230812 a36a5f0
Header files in filse using URL included
Replaced the Thor URL and URL-branch literals with DEFINEs
Header files in prg using URL included for ThorRepository
Replaced the ThorRepository URL and URL-branch literals with DEFINEs
Ignore files in "Updates", if identical named file is found in "My Updates"
working on VFPX#203
@Jimrnelson
Copy link
Collaborator

@lscheffler

I have cancelled this pull request for the following reasons:

[1] This is a solution to a problem that may never occur (at least in our lifetimes). While nothing lasts forever, my guess is that github is gonna be around for quite some time.

[2] Should this problem occur, there will be lots of work to do, and I think the most trivial part of that will be correcting the URLs. All references to them would have to be re-evaluated anyway. I see no need to make any initial pass at them now.

[3] If there is any value in fixing the URLs, I am not convinced that using #Defines and #Includes is the way to go and that it works well in the Thor environment. I am not saying that it's wrong, just that I would want to evaluate alternatives to make sure that it's the best approach. (I for one am not a fan of #Defines/#Includes and have concerns about how they would work in the Thor environment, most of which is in open PRGs, not APPs.)

[4] Should there come a time to implement this (and I see no compelling reason to do so), I certainly would not want to make a mass change to such a large number of Thor files. The time to adequately test all of this would be staggering and the risk of breaking Thor in the meantime would be considerable.

@Jimrnelson Jimrnelson closed this Dec 2, 2023
@lscheffler
Copy link
Contributor Author

It would have been possible to read the linked CONTRIBUTING, or simple look up the test provided. Or simple run a search tool like Code References over Thor to see where URL's are used at all. The strings "/Thor" and "/master" or "http" / "https" might do.

Nobody says blindly merge.

Pro tip: installed files\thor\tools\procs\thor_proc_check_for_updates.prg oops. A DEFINE for an URL ... Seems not only to be me to have this idea. Probably one might ask the person doing the change changed URL from VFPXRepository to GitHub about the fun doing this over and over - it even looks like this is a good search string to identify the places pointing to Thor or ThorRepository repo.

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