Fix deploy error and deploy test case #2346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tlylt
reviewed
Jul 24, 2023
tlylt
reviewed
Jul 25, 2023
tlylt
requested changes
Jul 29, 2023
There was a problem hiding this comment.
Thank you @WillCWX for working on this PR. I have a few comments. Main issues:
- Could you take another look at the original PR and the changes made? https://github.com/MarkBind/markbind/pull/2270/files
- I'm not sure why some changes (deletion, type assertions) that were made are not transferred back in this PR. I have pointed out a few places but I realized they are not done properly. If you have any concerns or comments as to why those are not necessary changes, please indicate for discussion.
- After everything is done, will need to re-organize into 2 commits as specified in the DG, one for rename, and one for migration. (Please rebase first as well)
|
I've compared the current migration with the previous PR and also the v5.0.0 file and made the changes accordingly. As requested, I've removed the comments noted and also all the |
|
If its ok, I'll merge with master to update the branch and then rebase into two commits as mentioned in the docs. |
|
Oh wait sorry let me do a final check |
|
👍 @WillCWX can organize and will merge after that's done |
Site/index.ts was previously bugged and caused errors with markbind deploy. As a stopgap measure, Site/index.ts was re-adapted back to JS. The bug has now been identified along with a fix, however, Site/index will need to be remigrated back to TS. The false negative test case in deploy has also been identifed and will be fixed as well. Let's remigrate Site/index to TS, fix the deploy option that caused the markbind deploy bug and also fix the negative test case that failed to detect the bug. Migration was made according to the original migration PR and the latest v5.0.0.
tlylt
approved these changes
Jul 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request?
Close #2333
Related to #2331
Overview of changes:
Site/index.jstoTypeScriptagain.options.remoteto bedefaultOptionConfig.remoteinstead of''options.remote = defaultDeployConfig.remoteingetDepUrlAnything you'd like to highlight/discuss:
No
Testing instructions:
set up a test repo that with github pages
run markbind deploy
Proposed commit message: (wrap lines at 72 characters)
Fix deploy error and deploy test case
Checklist: ☑️