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

Return expect to verify neo-cli #3318

Merged
merged 14 commits into from
Jun 14, 2024
Merged

Return expect to verify neo-cli #3318

merged 14 commits into from
Jun 14, 2024

Conversation

vncoelho
Copy link
Member

@vncoelho vncoelho commented Jun 10, 2024

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

Please put scripts and other development files in /.neo folder of repo. You can create directories or files in there. That's what it for.

@Jim8y
Copy link
Contributor

Jim8y commented Jun 11, 2024

Please put scripts and other development files in /.neo folder of repo. You can create directories or files in there. That's what it for.

So we can run the test locally?

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jun 11, 2024

So we can run the test locally?

Yes, organization and for csproj files or props can use if needed. Just a global place.

Jim8y
Jim8y previously approved these changes Jun 11, 2024
@vncoelho
Copy link
Member Author

Please put scripts and other development files in /.neo folder of repo. You can create directories or files in there. That's what it for.

.neo is not appropriate, a hidden folder.

I script, for now, a Script folder inside Neo.CLI itself.

@vncoelho vncoelho requested a review from cschuchardt88 June 11, 2024 11:46
Jim8y
Jim8y previously approved these changes Jun 11, 2024
Copy link
Member

Choose a reason for hiding this comment

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

We don't scripts included the csproj files. Just create a folder /scripts/Neo.CLI and put there. If doesn't make sense to put in with source files.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can fix it in another pr. If @vncoelho 's purpose is bring this back to the project. Please let him just bring it back.

Copy link
Member

Choose a reason for hiding this comment

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

I'm agree with do it organized, why we need to create one pr for one thing and other for move it? review is for this, the solution is not create a new pr.

Copy link
Contributor

@Jim8y Jim8y Jun 12, 2024

Choose a reason for hiding this comment

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

I'm agree with do it organized, why we need to create one pr for one thing and other for move it? review is for this, the solution is not create a new pr.

The problem is this pr from @vncoelho is to bring expert back, moving it somewhere is a request from @cschuchardt88 , which is not what @vncoelho wanted to do..... Why would you require/enforce @vncoelho to do a thing that is out of the scope of his pr? I am not against of making it organized, but what if @vncoelho just not willing to apply his suggestion? what we do with this pr then? close it and open another one?

So my point is very simple, one pr for only one task, if you agree that we should bring expert back, and @vncoelho 's pr has fullfilled this purpose, we should be fine with this pr. You can have suggestions, but that should not be a reason of enforcing unless there is a real problem in the pr.

From our past working experience, i believe having small prs (real pr that can fix problem without introducing new problem), is the only way we can make it forward.

BUT, its just my personal suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

His suggestion is good anyway, I will move to the main folder.

Copy link
Member Author

@vncoelho vncoelho Jun 12, 2024

Choose a reason for hiding this comment

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

Something I would like to highlighted is that @cschuchardt88 is using the Requested Changes too much.
I think that this is more useful when you are really blocking something for a good reason.
These uses like this are not necessary and just reduce the criteria we will often look to these requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm agree with do it organized, why we need to create one pr for one thing and other for move it? review is for this, the solution is not create a new pr.

The problem is this pr from @vncoelho is to bring expert back, moving it somewhere is a request from @cschuchardt88 , which is not what @vncoelho wanted to do..... Why would you require/enforce @vncoelho to do a thing that is out of the scope of his pr? I am not against of making it organized, but what if @vncoelho just not willing to apply his suggestion? what we do with this pr then? close it and open another one?

So my point is very simple, one pr for only one task, if you agree that we should bring expert back, and @vncoelho 's pr has fullfilled this purpose, we should be fine with this pr. You can have suggestions, but that should not be a reason of enforcing unless there is a real problem in the pr.

From our past working experience, i believe having small prs (real pr that can fix problem without introducing new problem), is the only way we can make it forward.

BUT, its just my personal suggestion.

I completely agree with you. This should be a common sense for most of the PRs.
Stop trying to block PRs for nonsense things that are not the goal of the PRs themselves.

Copy link
Member

Choose a reason for hiding this comment

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

I'm agree with do it organized, why we need to create one pr for one thing and other for move it? review is for this, the solution is not create a new pr.

The problem is this pr from @vncoelho is to bring expert back, moving it somewhere is a request from @cschuchardt88 , which is not what @vncoelho wanted to do..... Why would you require/enforce @vncoelho to do a thing that is out of the scope of his pr? I am not against of making it organized, but what if @vncoelho just not willing to apply his suggestion? what we do with this pr then? close it and open another one?

So my point is very simple, one pr for only one task, if you agree that we should bring expert back, and @vncoelho 's pr has fullfilled this purpose, we should be fine with this pr. You can have suggestions, but that should not be a reason of enforcing unless there is a real problem in the pr.

From our past working experience, i believe having small prs (real pr that can fix problem without introducing new problem), is the only way we can make it forward.

BUT, its just my personal suggestion.

Something I would like to highlighted is that @cschuchardt88 is using the Requested Changes too much. I think that this is more useful when you are really blocking something for a good reason. These uses like this are not necessary and just reduce the criteria we will often look to these requests.

Than you should be creating a Issue and assigning someone to that task. I can't keep being the repo cleaner upper. Put some thought into it, that's all. Just remember no one can do it the right way but yourself. No copy/paste and hope.

@NGDAdmin NGDAdmin merged commit a6dff23 into master Jun 14, 2024
8 checks passed
@NGDAdmin NGDAdmin deleted the verify-neo-cli-with-expect branch June 14, 2024 03:40
Jim8y added a commit to Jim8y/neo that referenced this pull request Jun 14, 2024
* master:
  Fixed pathing and Property (neo-project#3306)
  [Neo CLI New Feature] Allow custom plugins (neo-project#3295)
  Return expect to verify neo-cli (neo-project#3318)
  [Neo CLI Optimization] fix exception message (neo-project#3314)
  fix plugin download url (neo-project#3329)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants