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

Added WindowStyle #67

Merged
merged 4 commits into from
Apr 12, 2024
Merged

Added WindowStyle #67

merged 4 commits into from
Apr 12, 2024

Conversation

mrboring
Copy link
Contributor

@mrboring mrboring commented Apr 9, 2024

I've attempted to add this myself. I've tested it and it appears to work.

  • Added WindowStyle. Defaults to Hidden
  • Added to ShellConfig and ExecConfig
  • Added unit tests

This is my first attempt at updating a CE! Any feedback is welcome.

@CaptnCodr
Copy link
Owner

CaptnCodr commented Apr 10, 2024

Thank you for your PR.
I added some comments on the code for consistency.
Also, Command.fs:35 the line with WindowStyle = ProcessWindowStyle.Hidden, can be removed.

I would love to see some documentation in the README.md. 🙂

@mrboring
Copy link
Contributor Author

I added some comments on the code for consistency.

Should these be visible on the Files changed tab? If yes, I can't see them.

Also, Command.fs:35 the line with WindowStyle = ProcessWindowStyle.Hidden, can be removed.

Removed.

I would love to see some documentation in the README.md.

I've added an example. I've stated that WindowStyle would only work on Windows. Is this correct, or would it work on Mac and Linux? If not, is there an #if/#endif that should be applied?

I'm happy to add more if you think they are needed.

src/Fli.Tests/ExecContext/ExecCommandConfigureTests.fs Outdated Show resolved Hide resolved
src/Fli.Tests/ExecContext/ExecCommandConfigureTests.fs Outdated Show resolved Hide resolved
src/Fli.Tests/ExecContext/ExecConfigTests.fs Outdated Show resolved Hide resolved
src/Fli/CE.fs Outdated Show resolved Hide resolved
src/Fli/CE.fs Outdated Show resolved Hide resolved
@CaptnCodr
Copy link
Owner

Should these be visible on the Files changed tab? If yes, I can't see them.

Do you see the comments now?

@mrboring
Copy link
Contributor Author

Should these be visible on the Files changed tab? If yes, I can't see them.

Do you see the comments now?

Yes, I can see them now. Thanks. I'll get to work on them.

@CaptnCodr
Copy link
Owner

In the README.md (https://github.com/CaptnCodr/Fli/blob/main/README.md?plain=1#L242) there are these two tables with how to use this CEs.
Below that there are the explainations of the enums and DUs in Fli e.g. Fli.Shells. You could put Fli.WindowStyle cases with the hint that Hidden is the default, there.

@mrboring
Copy link
Contributor Author

In the README.md (https://github.com/CaptnCodr/Fli/blob/main/README.md?plain=1#L242) there are these two tables with how to use this CEs. Below that there are the explainations of the enums and DUs in Fli e.g. Fli.Shells. You could put Fli.WindowStyle cases with the hint that Hidden is the default, there.

Should Fli.Shells not be Fli.Domain.Shells?

@CaptnCodr
Copy link
Owner

Should Fli.Shells not be Fli.Domain.Shells?

Domain is being auto opened, it doesn't matter. It feels like it's directly in Fli. 🤫

@CaptnCodr
Copy link
Owner

Is this correct, or would it work on Mac and Linux? If not, is there an #if/#endif that should be applied?

According to microsoft docs, it's supported.

@mrboring
Copy link
Contributor Author

I think I've made the requested changes. Please let me know if I've missed anything.

README.md Outdated Show resolved Hide resolved
@mrboring
Copy link
Contributor Author

I've made the requested change to README.md.

@CaptnCodr CaptnCodr merged commit 363cd7f into CaptnCodr:main Apr 12, 2024
3 checks passed
@CaptnCodr
Copy link
Owner

That looks fine. You did good. 🙂
Thank you for your contribution. ❤️

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