-
Notifications
You must be signed in to change notification settings - Fork 164
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
One-Stop Config File for Code Portfolio #2161
Comments
I don't think we should alter the repo-config.csv file. It is optimized for situations where there is a large number of repos. What we need is a one-stop config file that can hold some basic config options of diverse nature, to cater for simpler cases. The specialized config files such as repo-config.csv need to remain for cases that need deeper configurations or to configure heavier datasets. |
Got it prof, I've updated the solution. |
@sopa301 a more suitable config file to repropose for this may be the |
Here's an example that makes sense from the user point of view, without being tied to JSON, YAML etc.:
|
Another thing: There is no strict need to force the entire file to be in one format. For example, if you look at the example above, you can split it first by |
#2192 Introduces blurb.md for specifying blurbs for repositories. It seems reasonable to me to include blurb specification to the one-stop config since users are likely to use the config for portfolios. This however would cause complications with different sources of truth for blurbs. There are number of ways we could deal with this:
Each have different pros/cons which perhaps we could discuss. |
I think option 3 would work best, considering the rest of the information in the one-stop config file can also be specified in |
This is true, we have the standalone config, CSVs and now the one-stop config all of which could conflict 😣. I think it might make sense even to find a way to deprecate the standalone config in place of the one-stop config. The interaction/priorities between the one-stop config and the CSVs could be the same as the interaction of the standalone config with the CSVs. |
Yes, #2171 is related to this. |
@ckcherry23 Thanks for linking that issue! It makes sense to me to do that together with this major new config, which has a lot of overlap in functionality |
Yup, we can expedite removing the standalone config file. I'm of two minds as to doing it in the same PR though. Reasons:
|
I personally feel introducing a third conflicting config, and ensuring that the logic for handling them (over handling precedence rules for only two) is done in a consistent way will be quite challenging and might cause some trouble when the logic is later removed when we remove the standalone config. Doing a direct swap will let us keep the existing precedence rules and logic between the standalone config and CSV. That being said, I do agree that removing the standalone config and introducing a fully featured new config in the same PR is a bit too large in scope. Perhaps we can break them down into 2 steps:
Step 1 will be breaking change however, and will require a major version bump. If this is undesirable, we could introduce the new config with the highest precedence and ignore all other configs if it's used (even if overriding), to reduce the implementation complexity. Given that the one-stop config is pretty fully featured, this behavior might be ok. |
That's workable as well. If we are starting from scratch, this is probably what we should do. But as we've already done a PR for the one-stop config file, the choice is not that clear. So, I'll leave you all to decide if it is easier to add the 'remove standalone config' to the current PR, or switch to the above 2-step approach (or some other strategy).
Bumping the major version is fine, if needed. There is no real cost of doing so. |
If I'm not mistaken, while #2192 introduces the new one-stop config and corresponding fields, not all of them are integrated into the analysis. It seems like currently, it replaces just the |
Hi @gok99, you are right in your assessment that the current implementation of the My intentions with the PR were to determine the correct file format and fields to include in the file; I was hoping that the format could be approved by everyone first before we start the process of writing the logic needed to meaningfully use the fields specified in the file to define configurations that will be used in the report. However, if you wish to combine both steps of converting from JSON to YAML and deprecating the standalone config files into one PR, I can edit the PR to include both steps instead! |
I think it would make sense then to do them in the same PR given the significant overlap between the configs. We can start by adding to the current one-stop config fields to subsume the standalone config - as far as I can tell, there are only two things that aren't already included
|
Perhaps we ignore the CSV files entirely if the one-stop file is present? There is no good reason for someone to have both formats, right?
Yes, this can be removed. repo level ignoreGlobList should be enough. |
Yes, the overriding behavior in CSVs no longer make much sense without the standalone config, and it's probably unlikely that users would need both configs - we can issue a warning if they are both specified. Removing all the overriding logic in #2192 would likely grow out of scope though, so we might just focus on:
Then, a second PR to properly remove the standalone config and the overriding columns in the CSVs.
👍On the point on the purpose of the one-stop config
It seems that we have now a pretty fully featured (almost) no-lose replacement of the CSV configs. This is nice because users can specify as much as they want and incrementally add to the config without needing to switch to the CSV. On the other hand, the specification for the one-stop config is no longer very basic. It could also get hard to explain which config should be used when and why two nearly identical configuration options exist. It might be a good idea at this stage to discuss if we want to keep all this customizability. |
Good point @gok99 We could start with a one-stop-config file that supports just enough attributes for an individual to set up their own code portfolio page using RepoSense (that use case was the motivation for this feature anyway). That is, we start with an MVP version of the feature first. What do you think? |
That sounds good to me. We could discuss what features to include. Here's a suggestion, based on the current one-stop config fields.
Fields removed:
Added a |
@gok99 @damithc I have updated the YAML file format to reflect the changes proposed in the above comment. As for the logic behind the one-stop config file, I will work on it incrementally over the coming month during my free time to get the application to a point such that, if specified, the one-stop config file takes precedence over the other CSV config files. If I'm not mistaken, I will most likely have to modify the |
@gok99 I'm not sure about this. In this 'personal code portfolio' use case, the user may have used just a few author names when contributing to the repo while the repo might contain commits from hundreds of other authors. Furthermore, new authors might join the repo continuously. So, feels like we should specify which authors to capture rather than which ones to ignore. |
On this, I'd rather give priority to the |
That makes sense! I think I removed it because authors have a number of different (potentially confusing) configuration options, but it seems reasonable to keep them.
I think it would be nice to have the same level of expressivity in the one-stop config as in the |
How about this?
That way, the user have the option to use either one but config file takes priority. |
Do you mean |
@gok99 That's fine too. What I don't want is the presence of the one-stop config file causing the blurbs.md to be ignored entirely. As long as I have the option to use the blurbs.md together with the one-stop config file, I'm OK with either of the two files taking priority over the other. |
If anyone would like to jump in to fix this issue, please feel free to! The only thing remaining is to finalise the priority in which to evaluate the config parameters and to implement it in code. |
What feature(s) would you like to see in RepoSense
Is the feature request related to a problem?
Currently, the format for configuring the one-stop config file
repo-config.csv
is difficult for inexperienced users to use. Additionally, it is not friendly for those trying to create a code portfolio.If possible, describe the solution
Repurpose
report-config.json
into a one-stop config file in a file format that is more user-friendly. Possible formats includeyml
andxml
. Additionally, include more configuration options (to be decided).This means that the order of priority would be this new file, then
repo-config
, then the rest of thecsv
files.Steps:
yml
and update the parser.Additional context
Expanded version of item 2 of #2157
The text was updated successfully, but these errors were encountered: