-
Notifications
You must be signed in to change notification settings - Fork 153
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
[#2161] One-Stop Config File for Code Portfolio #2192
base: master
Are you sure you want to change the base?
[#2161] One-Stop Config File for Code Portfolio #2192
Conversation
…implement-one-stop-config-file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on the YAML config file @asdfghjkxd! I have some questions and left them as comments below.
@@ -107,12 +107,22 @@ e.g.: `example.java` in `example-repo` can either be in the `test` group or the | |||
|
|||
<!-- ==================================================================================================== --> | |||
|
|||
## `report-config.json` | |||
## `report-config.yaml` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also update the --config
flag under 'CLI syntax reference' to include report-config.yaml
instead of json
? https://docs-2192-pr-reposense-reposense.surge.sh/ug/cli.html#config-c
Let's also update the 'Customize using CSV config files' heading on the 'Customizing Reports` page to include YAML or remove file types entirely. https://docs-2192-pr-reposense-reposense.surge.sh/ug/customizingReports.html#customize-using-csv-config-files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
public static final List<String> DEFAULT_FILE_FORMATS = List.of( | ||
"override:java", "md", "fxml" | ||
); | ||
public static final List<String> DEFAULT_IGNORE_GLOB_LIST = List.of( | ||
"docs**" | ||
); | ||
public static final List<String> DEFAULT_IGNORE_COMMITS_LIST = List.of( | ||
"2fb6b9b2dd9fa40bf0f9815da2cb0ae8731436c7", | ||
"c5a6dc774e22099cd9ddeb0faff1e75f9cf4f151", | ||
"cd7f610e0becbdf331d5231887d8010a689f87c7", | ||
"768015345e70f06add2a8b7d1f901dc07bf70582" | ||
); | ||
public static final List<String> DEFAULT_IGNORE_AUTHORS_LIST = List.of( | ||
"author1", | ||
"author2" | ||
); | ||
public static final boolean DEFAULT_IS_FIND_PREVIOUS_AUTHOR = false; | ||
public static final boolean DEFAULT_IS_SHALLOW_CLONING = true; | ||
public static final boolean DEFAULT_IS_IGNORE_STANDALONE_CONFIG = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we need these hardcoded default values here? Check for this in the other files inside reportconfig/
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe they are present for testing purposes (creating default instances), but I will have to check before reverting back to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they're just for tests, could we move them into the test files like we do for test defaults for the other classes? It doesn't seem like these values are useful defaults outside of the tests.
…sdfghjkxd/RepoSense into implement-one-stop-config-file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR complete? I don't see how this config is handled beyond being parsed and having the name read from in SummaryJson.java
. How does the config fit into the existing processing flow and modify RepoConfiguration
(along with the interaction with the conflicting CSV fields)?
title: RepoSense Report | ||
group-details: | ||
- repo: https://github.com/reposense/testrepo-Delta.git | ||
groups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not merge groups with the repo details in repos
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I closely followed the structure of the existing CSV files when modelling the config file, but on closer inspection, putting groups
into repos
might have been much cleaner! Will work on this in the upcoming weeks!
} | ||
|
||
@Override | ||
protected ReportConfiguration fromJson(Gson gson, Path path, Type type) throws IOException , JsonMappingException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fromYaml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially wanted to follow the same interface as JsonParser
, but in retrospect it might have been better to declare another abstract YamlParser
that implements similar interface methods as JsonParser
. I could probably work on this in the upcoming weeks if I have some time; anyone is welcome to jump in and refactor the code if this feature is needed urgently!
Part of #2161
Proposed commit message
Other information
Requires CI/CD to be updated to Java 11: