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

Support custom working directory input #57

Merged
merged 7 commits into from
Sep 30, 2022

Conversation

kendallroth
Copy link
Contributor

@kendallroth kendallroth commented Sep 21, 2022

  • Add custom working-directory Action input
  • Cleaned up README input documentation

Notes

Noticed that path.resolve is used both when getting the paths from Action inputs, as well as when actually reading files from disk. However, since path.resolve always returns an absolute path (using current working directory if no absolute path is provided), there is no need to use path.resolve later. Since jsonSummaryPath, jsonFinalPath, and viteConfigPath are already absolute paths once parsed/resolved, any functions using them technially do not need to try to resolve them again, as it will use the first absolute path provided (which is input path)... That being said, there is no harm in it either (due to shortcutting in path.resolve), so it's probably completely fine as is 🤷.

README.md Outdated Show resolved Hide resolved
@davelosert
Copy link
Owner

@kendallroth : Thank you so much for putting up this PR. I am going to have a deeper look at it later, but looks great at a first glance 👀 ❤️ .

For testing (your question in the issue): So far, I just tested this action directly in this repository with a workflow (see the last step in .github/workflows/test.yml, but not locally.

Act might be solution, but I have to give it some thought on how to actually test the output which is a comment in an issue. 🙂

For now, you could just add some coverage-files in a subfolder in the test-directory (e.g. test/example-subdirectory/.coverage and then add the following step to the above workflow:

    ...
    - name: 'Use this action'
      # run step also on failure of the previous step
      if: always()
      uses: ./
    - name: 'Use this action'
      # run step also on failure of the previous step
      if: always()
      uses: ./
      with:
        working-directory: ./test/example-subdirectory

Copy link
Owner

@davelosert davelosert left a comment

Choose a reason for hiding this comment

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

@kendallroth : Thank you so much for this PR. Overall, it looks pretty good so great job here! 👍🏻

I think it would be better to explicitly set the default value for the working directory, so it'd be great to change that.

Also as you might see, I am not a big fan of code-comments that describe WHAT the code is doing - so maybe we can get rid of some of those?

I also activated workflows to test the action - but it does not really run and throws a warning, so I still have to figure out why it is not working (this is the first outside-contribution, so might be a permissions problem).

README.md Outdated Show resolved Hide resolved
action.yml Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/parseJsonReports.ts Outdated Show resolved Hide resolved
src/parseJsonReports.ts Outdated Show resolved Hide resolved
src/parseJsonReports.ts Outdated Show resolved Hide resolved
src/parseThresholds.ts Outdated Show resolved Hide resolved
@kendallroth
Copy link
Contributor Author

Thanks for the review; have removed most of the comments and updated a few function names. I am still (after years...) trying to figure out my approach to comments. While I definitely agree that comments describing the "what" (or how?) of code should be avoided, I'm still trying to figure out when the "how" comments are useful to future devs (to avoid spending time trying to understand even built-in behaviour). However, at the end of the day it is a moot point, as this is your library and I fully respect your standards/conventions 🙂.

P.S. Ah, gotcha, I had completely missed the local test in Actions 👍.

@davelosert
Copy link
Owner

Haha no worries, I also have no hard stance on comments anymore (this used to be different 🙂). So I am completely open to them if they are helpful for other devs (which in this case is you) as I am of course a bit blind for them on my own code - but will still challenge them sometimes. 🙂

However, at the end of the day it is a moot point, as this is your library and I fully respect your standards/conventions 🙂.

Well, you are contributing to this now and spending your time and energy, so you definitely have say to it as well! Just because they are 'my' standards so far does not mean they are necessarily good standards that shouldn't be challenged / changed. 🙂

Last work on this PR

Regarding the PR, there are only two things left:

  1. Rename the parseThresholds.ts as by my comment
  2. Add some mock coverage reports to a folder (e.g. test/mockCoverageData/.coverage) and then use this path in an additional test to the .github/workflows/test.yml file.

I can take care of both so you don't have to invest more time into this (you did plenty already 🙏🏻), but I also don't want to take it away from you - just tell me.

@kendallroth
Copy link
Contributor Author

I have renamed the coverage threshold file, but am not 100% sure on the mock coverage reports (am completely fine if you take that to eliminate back and forth).

Copy link
Owner

@davelosert davelosert left a comment

Choose a reason for hiding this comment

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

So, the tests are still failing, but this is a permissions problem in the action I guess and has nothing to do with the code. I'd say this is ready to ship, but I want to test it once I released it and I don't have the time to do that tonight anymore.

But I will merge and release it tomorrow first thing in the morning.

Thank you so much again @kendallroth for this contribution. I will also put up a contributors file or something to make sure that you are mentioned 🙂

@kendallroth
Copy link
Contributor Author

No problem, I enjoyed helping out and am excited to keep using this Action going forward!

@davelosert davelosert merged commit cc4a408 into davelosert:main Sep 30, 2022
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants