Skip to content
This repository has been archived by the owner on May 31, 2018. It is now read-only.

Add file and function documentation #633

Merged
merged 3 commits into from
Apr 12, 2017

Conversation

ismaelgv
Copy link
Contributor

@ismaelgv ismaelgv commented Jan 29, 2017

Lets open this PR to work and discuss the documentation of the code.

Related to #443

The following list shows the tasks to do:

  • Document general script (pacaur)
  • Document information functions (libpacaur/aur.sh)
  • Document cache functions (libpacaur/cache.sh)
  • Document check functions (libpacaur/checks.sh)
  • Document dependency functions (libpacaur/deps.sh)
  • Document JSON functions (libpacaur/json.sh)
  • Document top level functions (libpacaur/main.sh)
  • Document package functions (libpacaur/pkgs.sh)
  • Document utility functions (libpacaur/utils.sh)

Commented functions may use makepkg style and include:

  • How it works
  • Usage
  • Arguments
  • Return values

For example (taken from makepkg - libmakepkg/util/option.sh.in):

##
# Checks to see if options are present in makepkg.conf or PKGBUILD;
# PKGBUILD options always take precedence.
#
#  usage : check_option( $option, $expected_val )
# return : 0   - matches expected
#          1   - does not match expected
#          127 - not found
##
check_option() {
# Actual function code
}

@ismaelgv
Copy link
Contributor Author

I've updated this PR description to add some tasks and indications.

@rmarquis I'm not sure about your idea about releasing version 5.0. Is there any additional task you want to complete besides documenting the code?

@rmarquis
Copy link
Owner

rmarquis commented Mar 23, 2017

@ChuckDaniels87 Some ideas were mentioned in #443:

  • Split of code
  • Reusing makepkg functions when possible
  • Maybe use of external libraries instead of in-house code (f.e. jq)

But above all: no new features or apparent changes. If something must be changed, this should occur either before or after pacaur 5 release.

@ismaelgv
Copy link
Contributor Author

We can use a GitHub project to organize all this work (see an example). On this PR, we can track all the documentation but we should create other PRs for tracking different tasks (splitting code, reusing makepkg, etc.).

@rmarquis
Copy link
Owner

@ChuckDaniels87 Added you as collaborator. Feel free to play with the new project board or push to the pacaur 5 branch. Please do not push changes to master prior to discussion, thx!

@ismaelgv
Copy link
Contributor Author

@rmarquis Thanks for the invitation! I'll try to figure out the best way to organize the workflow.

@ismaelgv ismaelgv added this to the 5.0.x - later milestone Mar 24, 2017
@ismaelgv
Copy link
Contributor Author

ismaelgv commented Mar 27, 2017

Before starting to document. Does pacaur use return values in any function? Or it just stores info in variables/arrays?

Edit: I mean other use than throwing generic errors. For example, use return value to identify the cause of the error and do something according to that.

@rmarquis
Copy link
Owner

Generic errors only.

@@ -3,6 +3,12 @@
# deps.sh - functions related to dependency resolution
#

##
# Dependency solver that wraps both pacman and AUR packages depedency
Copy link
Owner

Choose a reason for hiding this comment

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

typo here: "depedency"

Copy link
Contributor Author

@ismaelgv ismaelgv Apr 9, 2017

Choose a reason for hiding this comment

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

Fixed.

##
# Sort dependencies to ensure correct resolution order.
#
# usage: SortDepsAur( $aur_packages )
Copy link
Owner

Choose a reason for hiding this comment

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

Note #652 here. This would ideally be merged with FindDepsAur() - I just haven't took time to do it properly yet.

@ismaelgv
Copy link
Contributor Author

ismaelgv commented Apr 9, 2017

Not directly related to documentation but to code structure: I think declare -A jsoncache and trap Cancel INT probably should be in pacaur main script or at the begining of their own subscript.

@rmarquis
Copy link
Owner

rmarquis commented Apr 9, 2017

I do agree.

@ismaelgv
Copy link
Contributor Author

ismaelgv commented Apr 9, 2017

Then, do I move them to main script? Where do you consider that would be the right place?

Another question: why Core() function is called with ${aurpkgs[@]} arguments if inside this function those arguments are not used.

@rmarquis
Copy link
Owner

rmarquis commented Apr 9, 2017

Where do you consider that would be the right place?

Hmm, I think I'd actually put SetJson() in utils and Cancel() trap in the main pacaur script right after Version().
I have to see that having both a main pacaur script and a main.sh script seems weird to me. The latter sounds like a file that contains the functions we couldn't put somewhere else. Maybe merge them?

Why Core() function is called with ${aurpkgs[@]} arguments ..

Seems to be a leftover from some refactoring I did a long time ago. I'll clean this up in the master branch.

@ismaelgv
Copy link
Contributor Author

ismaelgv commented Apr 9, 2017

I have to see that having both a main pacaur script and a main.sh script seems weird to me. The latter sounds like a file that contains the functions we couldn't put somewhere else. Maybe merge them?

I think it is better to keep them separated. Main script pacaur already has many variable declarations, configuration and a lot of functionality outside of an actual function. main.sh just keep main functionality of pacaur in clean functions in a separate file. I think even more things can be extracted from main script a put them into the subscripts. Just my point of view, you know much better code workflow than me.

@ismaelgv
Copy link
Contributor Author

Hmm, I think I'd actually put SetJson() in utils and Cancel() trap in the main pacaur script right after Version().

I've been thinking about this. Moving SetJson() only to utils looks strange to me. What about moving all JSON related functions to utils?

Just an idea about Cancel() and other main functions. As I said before, I would move all functions from pacaur to main.sh, only keeping configuration and main block in there. What about moving Version() and Usage() also to main.sh. That gives code structure more sense and logic. If you don't want to make that change I agree with you that maybe keeping separate pacaur and main.sh looks strange.

@rmarquis
Copy link
Owner

Yes, keeping all Json related functions together is logical, and I guess utils is fine for them. Agree with the rest of your suggestion too.

The only function I don't like to see in main.sh is UpgradeAur() as it feels out of place. What about merging it in info.sh with InfoAur and SearchAur? The name of that subscript should also probably changed to something else, f.e. aurfunctions or operations.

@ismaelgv
Copy link
Contributor Author

Yes, keeping all Json related functions together is logical, and I guess utils is fine for them. Agree with the rest of your suggestion too.

I was wondering if a separated file (json.sh for example) would be better since there are many JSON functions and taking a closer look utils.sh I am not sure if they fit there. I'll wait your call because it is not clear to me.

The only function I don't like to see in main.sh is UpgradeAur() as it feels out of place. What about merging it in info.sh with InfoAur and SearchAur? The name of that subscript should also probably changed to something else, f.e. aurfunctions or operations.

I think this is perfect.

@rmarquis
Copy link
Owner

A specific json.sh file sounds good to me! 👍

@ismaelgv ismaelgv force-pushed the documentation branch 2 times, most recently from 5cf1971 to 03c42ea Compare April 10, 2017 23:31
@ismaelgv
Copy link
Contributor Author

ismaelgv commented Apr 10, 2017

I moved JSON functions to a new script and added vim config at the end of each file. I've rebased the pull request to avoid conflicts.

Any other change or addition that could be on this PR?

@rmarquis
Copy link
Owner

Don't think so, additional suggestion might come later on.

@ismaelgv
Copy link
Contributor Author

Should I merge it or just keep it open for some time if something new comes up?

@rmarquis
Copy link
Owner

Whichever you'd like, but I'd prefer to see the commits squashed together before merging. I don't think there is a need to have many commits that only do code reorganization and doc (just my opinion of course).

* Move IgnoreDepsChecks() from deps.sh to checks.sh.
* Minor fix in pkgs.sh
* Rename info.sh to aur.sh.
* Move JSON functions to a new script.
* Add vim config to all scripts
@ismaelgv
Copy link
Contributor Author

Reduced to 3 commits. 👍

@ismaelgv ismaelgv changed the title [WIP] Add file and function documentation Add file and function documentation Apr 12, 2017
@ismaelgv ismaelgv merged commit 9035f37 into rmarquis:pacaur50 Apr 12, 2017
@ismaelgv
Copy link
Contributor Author

ismaelgv commented Apr 12, 2017

I've merged this PR. I think it is better to open other PRs to different features and it will also avoid possible conflicts.

Edit: I've created a merge commit against my will. I'll try to fix it.
Edit2: Rebased and fixed.

@ismaelgv ismaelgv deleted the documentation branch April 12, 2017 16:25
@rmarquis
Copy link
Owner

Btw, did you intend to improve the documentation of the functions internally, rather than just their headers? I forgot about it, but I thought the main issue (for a third part eye) wasn't what the general idea the functions were doing, but the detail of it.

@ismaelgv
Copy link
Contributor Author

ismaelgv commented Apr 26, 2017

As a first approach, I tried to understand how program works globally. I intended to comment function internals in a separate PR. I'm pretty busy these days but I try to do it as soon as possible.

Edit: I've created a Card in the Project panel to not forget this.

@rmarquis
Copy link
Owner

All right, thanks for the clarification. No worries, there is need to hurry - I'm myself pretty busy.

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

Successfully merging this pull request may close these issues.

2 participants