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

Top VI Filtering #21

Closed
wants to merge 4 commits into from
Closed

Top VI Filtering #21

wants to merge 4 commits into from

Conversation

crossrulz
Copy link
Contributor

Resolved #16 - Only a VI or VIT that is "Standard" can be set to as the Top Level
Resolved #20 - The project owning the VI that Quick Drop is launched from is used instead of the application active project
Resolved #18 - Added "get" as an acceptable input to read the Top Level VI path
Resolved #19 - Added "get" to list of acceptable commands with an invalid input

Added "get" to the option to read what the Top Level VI is.
Added "get" to the acceptable items in the dialog.
@CaseyZS
Copy link
Collaborator

CaseyZS commented Dec 22, 2024

@crossrulz
Code:

The rest of the code good, but I have a couple of additional comments related to the process (congrats on being the guinea pig):

  1. I think you might run into issues syncing your repo with this one as soon as the new VIP is published because your commit history (specifically on your 'main' branch) is going to clash with this one. Moving forward, I think a way to avoid this is to give the feature branch a name other than one that already exists in this repo (e.g. "feature/issue16"). I'm not entirely sure what to do about it in this particular case, but I wanted to give you a heads-up.
  2. I think we should (at the very least) be documenting how we're testing functionality. I'm going to create a document in this 'develop' branch to track how testing was done and then developers can update it so any reviewers can reproduce the tests during the PR. Be on the lookout for that and you're going to want to merge this 'develop' into your development branch.
  3. This probably goes without saying and it's also mentioned in The VI being set as the Top Level VI needs to be a part of the project #20, but could you add any files you use for testing to the repo as well (e.g. VIs not a part of the project).

Again, thanks for being the test subject on this. Feel free to reach out if any of this wasn't clear.

@crossrulz
Copy link
Contributor Author

  1. I did not touch the vipb file size the last PR, so don't think this will cause a conflict in the immediate. I'll see about fixing what branch I am working off of in the future.
  2. I was also thinking about how to automate some of this testing. Some will need to be manual. But formalizing the testing process is a good idea.
  3. Any VIs and other types I used to test with should be in the Test folder. I will double check.

@CaseyZS
Copy link
Collaborator

CaseyZS commented Dec 22, 2024

@crossrulz
RE: number 1 - It's not the VIPB that I think would cause issues, it's the branch history. For an example, I simulated what the branch history would look like if I accepted your PR, created a new release branch and then finished it (using gitflow). I haven't pushed anything because this is just an example but look at what happens:
image

In this example, I renamed 'main' in your fork to 'crossrulz-main', but you can see that the histories of those two branches can't easily be synched since their commit histories can't be made to match (without more merging).

@crossrulz
Copy link
Contributor Author

I think the issue you are seeing is the crossrulz-main is currently linked to origin/main. I think I have a fix for this on my side (created a Develop branch that is directly linked to origin/develop). You'll see this when I update the PR after giving people a chance to try out the update due to the issue you found in #20.

@crossrulz
Copy link
Contributor Author

In order to clean up the branch history, I need to close this PR. I will then create another one using a branch that is directly forked from the Develop branch.

@crossrulz crossrulz closed this Jan 17, 2025
@crossrulz
Copy link
Contributor Author

Resubmitted as #23 using a branch forked from the Develop branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants