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

docs(website): Fix several inspections hints #7635

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

fviernau
Copy link
Member

@fviernau fviernau commented Oct 5, 2023

No description provided.

@fviernau fviernau requested a review from a team as a code owner October 5, 2023 09:49
@fviernau fviernau enabled auto-merge (rebase) October 5, 2023 09:49
@fviernau fviernau changed the title docs(website): Fix some several inspections hints docs(website): Fix several inspections hints Oct 5, 2023
|----------|-------------|
| deniedProcessEnvironmentVariablesSubstrings | A list of substrings that identify variables containing sensitive information. All variables that contain at least one of these strings (ignoring case) are not propagated to child processes. The default for this property contains strings like "PASS", "PWD", or "TOKEN", which are typically used to reference credentials. |
| allowedProcessEnvironmentVariableNames | This is a list of variable names that are explicitly allowed to be passed to child processes - even if they contain a substring listed in `deniedProcessEnvironmentVariablesSubstrings`. Via this property variables required by external tools, e.g. credentials for repositories needed by package managers, can be passed through. Here, entries must match variables names exactly and case-sensitively. |
| Property | Description |
Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken, these changes do not result in any visual differences. So, could you please elaborate in what way this is more "correct"? If this is just about aligning cell layout in the code, I tend towards keeping it as-is for less whitespace disturbance.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was flagged by a code inspection hint. This commit silences it.

Copy link
Member

Choose a reason for hiding this comment

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

Please provide the details in the commit message then (which tool was used, which is the rule that was violated). Esp. as none of our automated Markdown checks seem to cover this, we need to find an agreement on what tool recommendations to follow, and which not.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sschuberth commit message has been updated, can I keep it or shall I drop the commit?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to hear @mnonnenmacher's opinion on that as he has recently addressed a bunch of Markdown style issues. Personally, I have a slight tendency to drop it, as we'd otherwise need to do the same change for all Markdown tables for consistency, and I prefer to be able to rely on our automated Markdown checks (but maybe there's a rule that we could enable, if this change is desired).

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've split this out into a separate PR, so let's continue discussion there: #7648.

@@ -47,7 +47,7 @@ created run configuration to your needs, e.g. by adding an argument and options
## Testing

ORT uses [Kotest](https://github.com/kotest/kotest) as the test framework. For running tests and individual test cases
from the IDE, the [Kotest plugin](https://plugins.jetbrains.com/plugin/14080-kotest) needs to be installed. Afterwards,
from the IDE, the [Kotest plugin](https://plugins.jetbrains.com/plugin/14080-kotest) needs to be installed. Afterward,
Copy link
Member

Choose a reason for hiding this comment

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

Note that while "afterward" seems to be exclusively American English, "afterwards" is not exclusively British English, but also American English. Personally, I prefer "afterwards" over "afterward".

Copy link
Member Author

@fviernau fviernau Oct 5, 2023

Choose a reason for hiding this comment

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

From my research, I found that in AE the term "afterward" is preferred. To my understanding we stick to AE. Given that, it implies to me that "afterward" makes sense / should be preferred.

Do you disagree with this argumentation? If so why?

Copy link
Member

@sschuberth sschuberth Oct 5, 2023

Choose a reason for hiding this comment

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

It's correct that we should stick to AE, and I'd agree with the change if proof was provided that AE prefers "afterward" over "afterwards". So, please share the reference you found during your research.

Edit: I did some quick research myself, and both this and this seem to confirm that in AE "afterward" is more common, so I'm fine with that change.

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (681df5e) 68.03% compared to head (e0046aa) 68.03%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7635   +/-   ##
=========================================
  Coverage     68.03%   68.03%           
  Complexity     2023     2023           
=========================================
  Files           344      344           
  Lines         16727    16727           
  Branches       2372     2372           
=========================================
  Hits          11381    11381           
  Misses         4363     4363           
  Partials        983      983           
Flag Coverage Δ
funTest-docker 69.40% <ø> (ø)
funTest-non-docker 36.45% <ø> (ø)
test 35.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fviernau fviernau requested a review from sschuberth October 5, 2023 10:07
@fviernau fviernau force-pushed the website-code-inspection-fixes branch from 22bcd69 to 3238167 Compare October 5, 2023 10:33
@fviernau fviernau force-pushed the website-code-inspection-fixes branch from 3238167 to e0046aa Compare October 6, 2023 07:07
@fviernau fviernau merged commit 533c54f into main Oct 6, 2023
22 checks passed
@fviernau fviernau deleted the website-code-inspection-fixes branch October 6, 2023 07:57
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